apply two real items from second PR review round

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.
This commit is contained in:
Siddhant Medar
2026-04-15 13:48:17 -05:00
parent 4836e36f65
commit e94c5e0100
+16 -6
View File
@@ -138,11 +138,13 @@ definitions:
- git clone -q "https://x-token-auth:${BITBUCKET_CLONE_TOKEN}@bitbucket.org/${BITBUCKET_WORKSPACE}/code-review-agent.git" - 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. # Build the PR URL and run the agent.
# The || echo below only covers the python main.py call: # AI review is advisory for *runtime* failures: agent crashes,
# agent crashes, AWS STS errors, and runtime exceptions become # AWS STS errors, and Python exceptions become warnings via the
# warnings instead of hard failures. A failure in git clone or # || echo below (which only wraps the python main.py call).
# apt-get above is still a hard failure (rare in practice). # *Infrastructure/configuration* failures (missing BITBUCKET_CLONE_TOKEN,
# AI review is advisory, not a gate. # 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}" - 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)" - 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 if [ -z "$latest_tag" ]; then latest_tag="v0.0.0"; fi
echo "Latest tag: ${latest_tag}" 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//') 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) major=$(echo "$version" | cut -d. -f1)
minor=$(echo "$version" | cut -d. -f2) minor=$(echo "$version" | cut -d. -f2)
patch=$(echo "$version" | cut -d. -f3) patch=$(echo "$version" | cut -d. -f3)