From e94c5e01001c0adaaff7e298f0f622260011292c Mon Sep 17 00:00:00 2001
From: Siddhant Medar <smedar@aarete.com>
Date: Wed, 15 Apr 2026 13:48:17 -0500
Subject: [PATCH] apply two real items from second PR review round
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

1. Tag format validation in release-prod (MEDIUM, defensive).
   Before parsing major/minor/patch from the latest tag, assert it
   matches ^v[0-9]+\.[0-9]+\.[0-9]+$. Empirically tested: catches
   v1, v1.2, v1.2.3.4, v1.0.0-rc1, v1.2.3-beta+build, and still
   accepts v0.0.0 (the first-release fallback).

   Without this, bash arithmetic silently mangles non-semver tags
   into wrong results via its 'treat empty/non-numeric as 0' rule.
   Worst case: tag 'v1' → cut -d. -f2/-f3 both return '1' →
   minor bump produces v1.1.2 instead of v1.0.1. No visible error.

2. Rewrite the AI review comment to match actual behavior.
   The old comment said 'advisory, not a gate' but also admitted
   git clone / apt-get are hard failures, contradicting itself.
   The new comment makes the runtime vs. infrastructure distinction
   explicit: runtime failures (agent crash, STS errors, Python
   exceptions) become warnings via || echo; infrastructure failures
   (missing token, clone failure, apt-get failure) stay hard.

   This is a doc fix, not a code change. Deliberately preserving
   the hard-fail behavior on config errors because silent
   degradation of AI review is the worst outcome — feature
   disappears from CI with no signal.

Explicitly rejected from the second review (empirically verified):
- Gate logic 'fragile' claim: tested in bash, correct for all
  our hardcoded ALLOWED values.
- ROLLBACK_TAG -z check 'passes empty': tested, -z correctly
  catches empty strings. Reviewer has the semantics wrong.
- chmod 600 + set +x: our comment already documents chmod 600
  as cosmetic/scanner-silencing; set +x wouldn't help because
  Bitbucket's line-echo is runner-level, not bash set -x.
- cd/python refactor (previous round): already rebutted in
  commit 4836e36f via empirical bash AND-OR list tests.
- Option B for git clone wrapping: silent degradation of
  config errors is worse than the current loud-fail behavior.
---
 bitbucket-pipelines.yml | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/bitbucket-pipelines.yml b/bitbucket-pipelines.yml
index a8392dc..730cd2a 100644
--- a/bitbucket-pipelines.yml
+++ b/bitbucket-pipelines.yml
@@ -138,11 +138,13 @@ definitions:
           - git clone -q "https://x-token-auth:${BITBUCKET_CLONE_TOKEN}@bitbucket.org/${BITBUCKET_WORKSPACE}/code-review-agent.git"
 
           # Build the PR URL and run the agent.
-          # The || echo below only covers the python main.py call:
-          # agent crashes, AWS STS errors, and runtime exceptions become
-          # warnings instead of hard failures. A failure in git clone or
-          # apt-get above is still a hard failure (rare in practice).
-          # AI review is advisory, not a gate.
+          # AI review is advisory for *runtime* failures: agent crashes,
+          # AWS STS errors, and Python exceptions become warnings via the
+          # || echo below (which only wraps the python main.py call).
+          # *Infrastructure/configuration* failures (missing BITBUCKET_CLONE_TOKEN,
+          # git clone failures, apt-get failures) remain HARD failures by
+          # design — they indicate the pipeline setup itself is broken and
+          # need to be loud, not silently degraded into a stale warning.
           - export PR_URL="https://bitbucket.org/${BITBUCKET_WORKSPACE}/${BITBUCKET_REPO_SLUG}/pull-requests/${BITBUCKET_PR_ID}"
           - cd code-review-agent && python main.py "$PR_URL" || echo "WARNING - AI review step failed (non-blocking)"
 
@@ -372,9 +374,17 @@ pipelines:
               if [ -z "$latest_tag" ]; then latest_tag="v0.0.0"; fi
               echo "Latest tag: ${latest_tag}"
 
-            # Parse the tag into its three version components
+            # Parse the tag into its three version components.
+            # Validate strict semver first — bash arithmetic silently mangles
+            # non-semver tags (v1, v1.2, v1.2.3-rc1 etc.) into wrong results.
+            # Fail loudly with a clear error so nobody ships a bad release.
             - |
               version=$(echo "$latest_tag" | sed 's/^v//')
+              if ! [[ "$version" =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; then
+                echo "BLOCKED: latest tag '$latest_tag' is not strict semver (expected v#.#.#)."
+                echo "Fix by creating a properly-formatted tag: git tag -a vX.Y.Z -m 'Release vX.Y.Z'"
+                exit 1
+              fi
               major=$(echo "$version" | cut -d. -f1)
               minor=$(echo "$version" | cut -d. -f2)
               patch=$(echo "$version" | cut -d. -f3)