diff --git a/docs/ci-workflow.md b/docs/ci-workflow.md index 32e0024..8c99945 100644 --- a/docs/ci-workflow.md +++ b/docs/ci-workflow.md @@ -19,7 +19,7 @@ Monday 08:00 ──► securityhub-summary Lambda ──► #javabin-infra-ale - Uploads `tfplan` + `plan-output.txt` to S3 with SHA256 hash - Posts plan output as PR comment on pull requests - Runs `scripts/review-plan.py` inline — calls Bedrock (Claude Haiku 5.5) for risk analysis -- The model gets the whole plan with refresh/read lines stripped; there is no input cap (Haiku 5.5 has a 1M-token context). The prompt says the plan is complete, gives its change count and `Plan:` line, and explains Terraform's own elision markers (`# (N unchanged attributes hidden)`, `(sensitive value)`, `(known after apply)`, partial `jsonencode` diffs) so the model does not report them as truncation. `REVIEW_MAX_PLAN_CHARS` can optionally set a cap: a plan above it gets a list of every resource change plus as much diff as fits, cut at a resource boundary, and the prompt says exactly which diffs are missing. A response with `stopReason: max_tokens` is discarded (FAILED / retried) rather than trusted. +- The model gets the whole plan with refresh/read lines stripped; there is no practical input cap (Haiku 5.5 has a 1M-token context): the default ceiling is 2M chars, 2.4x the largest real plan. The prompt says the plan is complete, gives its change count and `Plan:` line, and explains Terraform's own elision markers (`# (N unchanged attributes hidden)`, `(sensitive value)`, `(known after apply)`, partial `jsonencode` diffs) so the model does not report them as truncation. `REVIEW_MAX_PLAN_CHARS` overrides the ceiling. A plan above it gets a list of every resource change plus as much diff as fits, cut at a resource boundary, and the prompt says exactly which diffs are missing. A response with `stopReason: max_tokens` is discarded (FAILED / retried) rather than trusted. - The PR comment shows at most 60k chars of the plan (GitHub's comment limit) and says so when it cuts; this does not affect the review. - Structured output via Bedrock Converse tool use: `{risk: LOW|MEDIUM|HIGH, summary: ..., findings: [...]}` - Posts review as PR comment diff --git a/scripts/post-plan-comment.sh b/scripts/post-plan-comment.sh index 957fb28..363a14d 100644 --- a/scripts/post-plan-comment.sh +++ b/scripts/post-plan-comment.sh @@ -33,6 +33,7 @@ PLAN_CLEAN_FILE="$(dirname "$PLAN_FILE")/plan-clean.txt" if [ ! -f "$PLAN_CLEAN_FILE" ]; then # Fallback: filter inline PLAN_CLEAN_FILE=$(mktemp) + trap 'rm -f "$PLAN_CLEAN_FILE"' EXIT grep -v -E '^(module\.|data\.|\s+#).*: (Refreshing state|Reading|Read complete)' "$PLAN_FILE" > "$PLAN_CLEAN_FILE" 2>/dev/null || true fi PLAN=$(head -c "$PLAN_COMMENT_CHARS" "$PLAN_CLEAN_FILE") diff --git a/scripts/review-plan.py b/scripts/review-plan.py index 41e2cda..c0098c6 100755 --- a/scripts/review-plan.py +++ b/scripts/review-plan.py @@ -25,10 +25,12 @@ # 1M-token context; plan text is roughly 3-4 characters per token). A fixed # character cap is what kept cutting real plans - first 80k, then 400k cut a # 231-destroy plan (830k chars; destroy diffs print every attribute) to 28 of -# its changes. REVIEW_MAX_PLAN_CHARS can still set a cap; a plan above it gets -# a complete change list plus as much diff as fits (see build_plan_section), -# never a silent cut. -MAX_PLAN_CHARS = int(os.environ["REVIEW_MAX_PLAN_CHARS"]) if os.environ.get("REVIEW_MAX_PLAN_CHARS") else None +# its changes. The default ceiling (2M chars, ~500-800k tokens) is 2.4x the +# largest real plan, so real plans are sent whole; only a pathological plan +# that couldn't fit the model at all gets a complete change list plus as much +# diff as fits (see build_plan_section), never a silent cut. +# REVIEW_MAX_PLAN_CHARS overrides it. +MAX_PLAN_CHARS = int(os.environ.get("REVIEW_MAX_PLAN_CHARS") or 2_000_000) # State refresh / data source read lines: noise for review purposes. Resource # addresses start in column 0; diff content is always indented. @@ -239,7 +241,7 @@ def review_plan(plan_text, converse=cli_converse): "# (N unchanged elements hidden)": for an update, Terraform shows only the changed attributes. The hidden ones are not changing. - "(sensitive value)": the value is redacted on purpose. -- "(known after apply)": the value is computed by AWS during apply. +- "(known after apply)": the value is not known until apply (computed by AWS or derived from other resources in this plan); judge it from the expression, not from a value. - jsonencode(...) policy diffs show only the changed statements and fields, so a changed statement can appear without its Effect, Resource or Sid. In a changed IAM policy statement an unchanged `Effect` is usually not shown, so diff --git a/scripts/tests/test_review_plan.py b/scripts/tests/test_review_plan.py index af7d04e..aee3247 100644 --- a/scripts/tests/test_review_plan.py +++ b/scripts/tests/test_review_plan.py @@ -133,13 +133,20 @@ def test_prompt_explains_terraform_elision_markers_are_not_truncation(self): self.assertIn(marker, prompt) self.assertIn("not truncation", prompt) - def test_no_input_cap_by_default(self): - self.assertIsNone(rp.MAX_PLAN_CHARS) - text = plan(20_000) - self.assertGreater(len(text), 2_000_000) + def test_default_ceiling_sends_real_plans_whole(self): + self.assertEqual(rp.MAX_PLAN_CHARS, 2_000_000) + text = plan(8_000) # ~0.9M chars: larger than any real plan so far + self.assertLess(len(text), rp.MAX_PLAN_CHARS) section = rp.build_plan_section(text) self.assertIn("COMPLETE PLAN", section) - self.assertIn("aws_iam_role_policy.p19999 will be updated in-place", section) + self.assertIn("aws_iam_role_policy.p7999 will be updated in-place", section) + + def test_pathological_plan_falls_back_to_change_list(self): + text = plan(20_000) + self.assertGreater(len(text), rp.MAX_PLAN_CHARS) + section = rp.build_plan_section(text) + self.assertNotIn("COMPLETE PLAN", section) + self.assertLessEqual(len(section), rp.MAX_PLAN_CHARS) def test_large_destroy_plan_is_sent_whole(self): # PR #135 (2026-10-24): 231 destroys, 830k chars, was cut to 28 changes by the old 400k limit.