APPROVED : Deployment | Add webhook notification

[MykolaMS9]

Summary

• Added ::add-mask:: for all secret variables in each step (SSH_PRIVATE_KEY, SSH_PUBLIC_KEY, GOOGLE_APPLICATION_PROJECT_ID, GOOGLE_APPLICATION_REGION, GOOGLE_ENCRYPTION_KEY, HOST_SERVER_NAME, HOST_SERVER_IP, HETZNER_CLOUD_TOKEN) — prevents runner logs from exposing values that GitHub does not automatically mask
• Replaced cat <<EOF with individual printf ... >> .secret/-secret.sh lines — safer handling of values containing special characters
• Secret files (service_account.json, ssh_key, ssh_key.pub) are now written to hardcoded .secret/ paths instead of ${{ secrets.*_PATH }} — eliminates the mismatch between write steps and cleanup
• Removed dependency on path secrets (GOOGLE_SE_CREDS_PATH, SECRET_RSA_*_KEY_PATH) — reduces the required secrets list
• Added optional Notify Google Chat on failure step via GCHAT_WEBHOOK_URL secret

Test plan

• Verify that secret values do not appear in runner logs
• Verify that .secret/service_account.json, .secret/ssh_key, .secret/ssh_key.pub are created and shredded correctly after deployment
• Verify Google Chat notification is received on failure when GCHAT_WEBHOOK_URL is configured
• Verify deployment succeeds without GCHAT_WEBHOOK_URL set (step must exit silently)

[wanguardd]

System of Review completed analysis of 'Deployment | Add webhook notification'. Found 4 findings across 2 files.

📋 Review Summary

Category	Count
🛡️ SECURITY	2
📐 DESIGN	1
🏛️ ORGANIZATIONAL	1

---

🛡️ SECURITY

[🔴 Blocking | INTRODUCED] Webhook notification constructs JSON by directly embedding ${{ github.ref_name }} and ${DEPLOYMENT_MODE} into a shell double-quoted string without JSON escaping. GitHub Actions evaluates ${{ }} expressions before the shell runs, inserting the raw value verbatim into the script. If a branch name contains \ (backslash — valid in git refs) or other special characters, the curl -d "{ ... }" argument becomes malformed JSON and the Google Chat API call silently fails or sends garbled content. The ${DEPLOYMENT_MODE} variable is also unescaped; a repository administrator setting it to a value with " or \ would silently break the notification. GitHub's own security guidance recommends against direct expression interpolation into shell command arguments.

Evidence:

# deploy.yaml:129-133
curl -s -X POST "${GCHAT_WEBHOOK_URL}" \
  -H 'Content-Type: application/json' \
  -d "{
    \"text\": \"❌ *Deploy failed*\n*Repo:* ${{ github.repository }}\n*Branch:* ${{ github.ref_name }}\n*Mode:* ${DEPLOYMENT_MODE}\n*Run:* ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}\"
  }"
# ${{ github.ref_name }} is resolved to the branch name before bash runs.
# A branch like feat/test\ produces \"*Branch:* feat/test\",
# which is invalid JSON (unescaped backslash in JSON string).

Action:

• Capture expression values as env vars (REF_NAME: ${{ github.ref_name }}) and construct JSON with jq: MESSAGE=$(jq -n --arg t "❌ *Deploy failed*\n*Repo:* ${GITHUB_REPOSITORY}\n*Branch:* ${REF_NAME}\n*Mode:* ${DEPLOYMENT_MODE}\n*Run:* ${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}/actions/runs/${GITHUB_RUN_ID}" '{text: $t}') && curl -s -X POST "${GCHAT_WEBHOOK_URL}" -H 'Content-Type: application/json' --data "$MESSAGE". This ensures all values are JSON-escaped regardless of content.

---

[🟡 Suggestion | INTRODUCED] Decoded GCP service account JSON is never masked in runner logs. echo "::add-mask::${GCP_CREDS_B64}" masks only the base64-encoded blob. After printf '%s' "${GCP_CREDS_B64}" | base64 --decode, the result is the raw service account JSON containing private_key, client_id, and client_email — a different string that is never registered with ::add-mask::. If a subsequent step produces error output that echoes part of the credential content, those fields would appear unmasked.

Evidence:

# deploy.yaml:34 — masks the base64 blob only
echo "::add-mask::${GCP_CREDS_B64}"
# deploy.yaml:35 — decoded JSON written to disk, never masked
printf '%s' "${GCP_CREDS_B64}" | base64 --decode > .secret/gcp_creds.json
# No ::add-mask:: for the decoded private_key, client_email, etc.

Action:

• After decoding, extract and mask the private_key_id field: PRIVATE_KEY_ID=$(jq -r '.private_key_id' .secret/gcp_creds.json) && echo "::add-mask::${PRIVATE_KEY_ID}". This masks the most sensitive identifier without attempting to mask the entire multi-line JSON blob (which ::add-mask:: cannot reliably do for multi-line values).

---

📐 DESIGN

[🟡 Suggestion | INTRODUCED] CI writes runtime secret files using names that conflict with the project's documented naming convention. The workflow creates .secret/gcp_creds.json, .secret/ssh_key, and .secret/ssh_key.pub. The project's .secret/readme.md documents these files as -service_account.json, -id_ed25519, and -id_ed25519.pub respectively, following the project-wide convention that hyphen-prefixed names are runtime/ephemeral files. This creates two separate naming schemes for the same files — one for CI automation and one for local development — which will confuse developers cross-referencing the secrets documentation with CI log output.

Evidence:

# .secret/readme.md lines 31-33 (documented convention)
|- -service_account.json      # GCP service account key
|- -id_ed25519                # SSH private key (ed25519)
|- -id_ed25519.pub            # SSH public key (ed25519)

# deploy.yaml:35, 50, 53 (CI-actual)
printf '%s' "${GCP_CREDS_B64}" | base64 --decode > .secret/gcp_creds.json
printf '%s\n' "${SSH_PRIVATE_KEY}" > .secret/ssh_key
printf '%s\n' "${SSH_PUBLIC_KEY}" > .secret/ssh_key.pub

Action:

• Either (a) align CI filenames with the documented convention by renaming to -service_account.json, -id_ed25519, -id_ed25519.pub in the workflow (lines 35, 50, 53, 97–99, and 141–148 in deploy.yaml); or (b) update .secret/readme.md to explicitly document the dual-context naming — noting that CI uses gcp_creds.json/ssh_key/ssh_key.pub while local dev uses the hyphen-prefixed forms.

---

🏛️ ORGANIZATIONAL

[🟡 Suggestion | INTRODUCED] .github/workflows/readme.md uses a code block for the directory listing rather than the Responsibility Table format required by the project's organizational governance standards. The ## Directory Structure section renders a tree diagram with ├── characters inside a fenced code block. The project's governance rulebook requires that any directory readme with 3+ files use a | File | Responsibility | Responsibility Table. .github/workflows/ now contains 4 permanent files.

Evidence:

# .github/workflows/readme.md lines 8-13
.github/workflows/
├── deploy.yaml                        # Production deploy (push to master / manual)
├── deploy-check.yml                   # Pre-merge deploy validation (PRs to master)
├── iron_token_manager_validation.yml  # iron_token_manager module tests & schema checks
└── readme.md

This code block should be a | File | Responsibility | table. The .github/workflows/ directory has 4 files, meeting the 3+ threshold for mandatory Responsibility Table documentation.

Action:

• Replace the ## Directory Structure code block with a ### Responsibility Table section:

  ### Responsibility Table
  
  | File | Responsibility |
  |------|----------------|
  | `deploy.yaml` | Deploy to Hetzner on push to master or manual trigger |
  | `deploy-check.yml` | Validate deploy infrastructure on PRs to master |
  | `iron_token_manager_validation.yml` | Run iron_token_manager tests and schema checks |
  | `readme.md` | Document workflow directory structure and usage |

---

This is a solid PR — the security hardening via ::add-mask:: for all secrets, the printf-based format for safe special-character handling, and the observability addition are all good improvements. Addressing the JSON safety issue in the webhook step and aligning the file naming with project conventions will bring it to merge-ready state.

---

❗ > Note: This System of Review operates with the same information access and constraints as any human developer working on this repository. While the system is highly accurate (99%+ reliability), any confusion or apparent misunderstanding typically indicates gaps in repository documentation, unclear code organization, or insufficient project discipline — the same issues that would impact any team member. If the System cannot locate critical information and draws incorrect conclusions, a human developer would face identical challenges. We maintain high standards to respect our teammates' time, our clients' investment, and the integrity of this project. Please ensure the repository provides clear, discoverable context for all reviewers.

Review approval is a quality floor, not a quality ceiling. It confirms that identified issues are resolved — not that the code is free of all issues. The System reviews the changes in this PR, not the entire codebase. Proactive quality is a developer responsibility: unit test coverage, edge case analysis, and consistency with the surrounding codebase cannot be delegated to any review process. If the review surfaces one instance of a pattern, the developer is responsible for auditing all occurrences of that pattern throughout the codebase — not just the specific line cited.

When addressing review feedback: Open a separate commit for each point you address, clearly referencing the finding in the commit message. This ensures knowledge is captured in the repository history and helps other developers avoid the same pitfalls. If you cannot address a point, leave a detailed comment in this PR review thread explaining specifically what is wrong with the finding or why it cannot be addressed — never ignore feedback silently. Use comments only in rare cases when the System has genuinely missed existing context in the repository; prefer commits as the primary response mechanism to build institutional knowledge.

[wanguardd]

System of Review completed analysis of 'Deployment | Add webhook notification'. Found 8 findings across 2 files.

📊 Progress Since Review #2

Changes since last review: 1 commit — chore (workflow): Incorporate the feedback from the review.

Previous review found 4 findings. The author resolved 3 of them in the feedback commit:

• ✅ JSON injection: jq -n --arg with env-var-captured GH expressions — fixed
• ✅ File naming convention: -service_account.json, -iron_sdk, -iron_sdk.pub — fixed
• ✅ Responsibility Table: code block replaced with proper | File | Responsibility | table — fixed
• 🔄 GCP JSON content masking — not addressed (re-raised below)

This review found 2 new ORGANIZATIONAL findings introduced by the readme and 5 carry-forward / newly surfaced items.

---

📋 Review Summary

Category	Count
🏛️ ORGANIZATIONAL	2
🛡️ SECURITY	2
🐛 CORRECTNESS	2
📐 DESIGN	2

---

🏛️ ORGANIZATIONAL

[🔴 Blocking | INTRODUCED] .github/workflows/readme.md uses ## (H2) headers throughout. The project documentation rulebook explicitly forbids H2: all sections must use H1 (document title) or H3 only.

Evidence:

# .github/workflows/readme.md — lines 5, 16 (current state)
## Directory Structure        ← H2, forbidden
## Workflows                  ← H2, forbidden

Action:

• Change both ## headings to ###:
  • Line 5: ## Directory Structure → ### Directory Structure
  • Line 16: ## Workflows → ### Workflows

---

[🔴 Blocking | INTRODUCED] .github/workflows/readme.md has no ### Scope section. The project structure rulebook mandates a Scope section for every directory readme that covers 3 or more permanent entities. .github/workflows/ now contains 4 permanent files (deploy.yaml, deploy-check.yml, iron_token_manager_validation.yml, readme.md), meeting the threshold.

Evidence:

# .github/workflows/ — current permanent files
$ ls .github/workflows/
deploy.yaml  deploy-check.yml  iron_token_manager_validation.yml  readme.md
# Count: 4 files → Scope section mandatory
# readme.md: no "### Scope" heading present

Action:

• Add a ### Scope section after the document title and before ### Directory Structure:

  ### Scope
  
  **Responsibilities:** Define and maintain all CI/CD automation for the Iron Cage SDK.
  
  **In Scope:** Workflow definitions for deployment, pre-merge validation, and module-specific checks.
  
  **Out of Scope:** Deployment scripts (`deploy/`), Makefiles, Terraform configuration, and manual release procedures.

---

🛡️ SECURITY

[🟡 Suggestion | EXISTING — RE-REQUEST] The Decode service account credentials step masks only the base64-encoded blob (${GCP_CREDS_B64}), not the decoded JSON content. After base64-decode, .secret/-service_account.json contains a raw private_key and client_email — different strings that are never registered with ::add-mask::. If any subsequent step (e.g., a verbose make deploy error path) echoes part of the decoded file, those fields appear unmasked in the runner log. This was raised in the previous review and was not addressed in the feedback commit.

Evidence:

# deploy.yaml:36-37
echo "::add-mask::${GCP_CREDS_B64}"        # ← masks base64 blob only
printf '%s' "${GCP_CREDS_B64}" | base64 --decode > .secret/-service_account.json
# No ::add-mask:: for the decoded private_key, client_email, private_key_id

Action:

• After decoding, extract and mask the sensitive identifier fields:

  echo "::add-mask::$(jq -r '.private_key_id' .secret/-service_account.json)"
  echo "::add-mask::$(jq -r '.client_email' .secret/-service_account.json)"

  Note: masking the full multi-line private_key value is not reliably supported by ::add-mask::; masking the private_key_id (a single-line UUID-like identifier) is the pragmatic safe minimum.

---

[🟡 Suggestion | INTRODUCED] printf 'KEY="%s"\n' "${VALUE}" embeds the value inside double quotes. If a secret value contains a literal " character or an embedded newline, the resulting line in .secret/-secret.sh becomes syntactically invalid shell — either the quote terminates early or a multiline key breaks Make's include parser. ALLOWED_ORIGINS (comma-separated URLs) and DATABASE_URL are the most plausible sources of unexpected characters.

Evidence:

# deploy.yaml:121 — example
printf 'ALLOWED_ORIGINS="%s"\n' "${ALLOWED_ORIGINS}" >> .secret/-secret.sh
# If ALLOWED_ORIGINS = 'https://a.com,"https://b.com"'
# Output: ALLOWED_ORIGINS="https://a.com,"https://b.com""   ← invalid shell

Action:

• Add a pre-write validation step checking no secret value contains " or \n, or escape values before writing:

  # Minimal guard (run before the printf block):
  for VAR_CHECK in "${ALLOWED_ORIGINS}" "${DATABASE_URL}"; do
    if printf '%s' "${VAR_CHECK}" | grep -qP '"|\n'; then
      echo "ERROR: Secret value contains illegal character (quote or newline)"
      exit 1
    fi
  done

---

🐛 CORRECTNESS

[🟡 Suggestion | INTRODUCED] if: failure() does not fire when a job is cancelled (via GitHub UI, timeout, or concurrency group displacement). A deployment that times out at 45 minutes or is manually cancelled produces no Google Chat notification. Production cancellations are operationally indistinguishable from no-op runs.

Evidence:

# deploy.yaml:131
- name: Notify Google Chat on failure
  if: failure()          ← cancelled() returns false; no notification sent
  continue-on-error: true

Action:

• Change the condition to cover cancellation:

  if: failure() || cancelled()

---

[🟡 Suggestion | INTRODUCED] HOST_SERVER_TYPE defaults are inconsistent across the deployment infrastructure. The Makefile and Terraform variables default to cx23; the .secret/secret.template.sh documents cx33; deploy/readme.md shows cx22. If vars.HOST_SERVER_TYPE is not configured in the repository, Make silently provisions a cx23 server — which may differ from the operator's expectation set by the template.

Evidence:

# deploy/Makefile.deploy
HOST_SERVER_TYPE := $(or $(call strip_quotes,$(HOST_SERVER_TYPE)),"cx23")

# deploy/hetzner_server_create/variables.tf
default = "cx23"

# .secret/secret.template.sh
HOST_SERVER_TYPE="cx33"

# deploy/readme.md
HOST_SERVER_TYPE="cx22"

Action:

• Align all default values to a single canonical type. The template (cx33) is the most authoritative reference for production intent — update Makefile.deploy and variables.tf defaults to match, or document the intended value explicitly in all locations.

---

📐 DESIGN

[🟡 Suggestion | INTRODUCED — RE-REQUEST] The notification step uses jq to construct the webhook payload. jq is pre-installed on GitHub-hosted runners but is not guaranteed on self-hosted runners. The workflow runs on ${{ vars.GH_RUNNER_DEPLOY }} — a self-hosted runner. With continue-on-error: true in place, a missing jq binary causes the notification step to fail silently, defeating its purpose. This situation is made worse by the step being the only failure-visibility mechanism.

Evidence:

# deploy.yaml:19, 142
runs-on: ${{ vars.GH_RUNNER_DEPLOY }}   ← self-hosted runner
...
jq -n \                                  ← no existence check; absent jq = silent failure
  --arg repo "${GH_REPOSITORY}" \

Action:

• Add a jq presence check or ensure it is in the runner provisioning playbook:

  command -v jq > /dev/null || { echo "ERROR: jq not found on runner"; exit 1; }

  Alternatively, document jq as a runner requirement in the deploy infrastructure readme.

---

[🟡 Suggestion | INTRODUCED — RE-REQUEST] The workflow writes SSH key files as .secret/-iron_sdk / .secret/-iron_sdk.pub. The .secret/secret.template.sh — the reference for local developer setup — documents these as SSH_PRIVATE_KEY_PATH=".secret/-iron_site" / .secret/-iron_site.pub". A developer following the template to set up a local .secret/ directory will use -iron_site; the workflow uses -iron_sdk. Developers cross-referencing CI log output against local configuration will see different filenames with no explanation.

Evidence:

# deploy.yaml:51-55 (CI produces)
.secret/-iron_sdk
.secret/-iron_sdk.pub

# .secret/secret.template.sh (template documents)
SSH_PRIVATE_KEY_PATH=".secret/-iron_site"
SSH_PUBLIC_KEY_PATH=".secret/-iron_site.pub"

Action:

• Either (a) update .secret/secret.template.sh to use -iron_sdk / -iron_sdk.pub to match CI; or (b) if local dev intentionally uses different key files, add a comment in the template and in readme.md explaining the divergence.

---

Strong progress on this iteration — the three most structurally important fixes (JSON injection, file naming convention, Responsibility Table) are well-executed. The remaining two blocking items are both in readme.md and are straightforward edits.

---

❗ > Note: This System of Review operates with the same information access and constraints as any human developer working on this repository. While the system is highly accurate (99%+ reliability), any confusion or apparent misunderstanding typically indicates gaps in repository documentation, unclear code organization, or insufficient project discipline — the same issues that would impact any team member. If the System cannot locate critical information and draws incorrect conclusions, a human developer would face identical challenges. We maintain high standards to respect our teammates' time, our clients' investment, and the integrity of this project. Please ensure the repository provides clear, discoverable context for all reviewers.

Review approval is a quality floor, not a quality ceiling. It confirms that identified issues are resolved — not that the code is free of all issues. The System reviews the changes in this PR, not the entire codebase. Proactive quality is a developer responsibility: unit test coverage, edge case analysis, and consistency with the surrounding codebase cannot be delegated to any review process. If the review surfaces one instance of a pattern, the developer is responsible for auditing all occurrences of that pattern throughout the codebase — not just the specific line cited.

When addressing review feedback: Open a separate commit for each point you address, clearly referencing the finding in the commit message. This ensures knowledge is captured in the repository history and helps other developers avoid the same pitfalls. If you cannot address a point, leave a detailed comment in this PR review thread explaining specifically what is wrong with the finding or why it cannot be addressed — never ignore feedback silently. Use comments only in rare cases when the System has genuinely missed existing context in the repository; prefer commits as the primary response mechanism to build institutional knowledge.

[MykolaMS9]

Done in commit
chore (workflow): Incorporate the feedback from the review n.2
751beb7

• Change both ## headings to ###:
  • Line 5: ## Directory Structure → ### Directory Structure
  • Line 16: ## Workflows → ### Workflows
• Add a ### Scope section after the document title and before ### Directory Structure:
• After decoding, extract and mask the sensitive identifier fields:

  echo "::add-mask::$(jq -r '.private_key_id' .secret/-service_account.json)"
  echo "::add-mask::$(jq -r '.client_email' .secret/-service_account.json)"

  Note: masking the full multi-line private_key value is not reliably supported by ::add-mask::; masking the private_key_id (a single-line UUID-like identifier) is the pragmatic safe minimum.
• Add a pre-write validation step checking no secret value contains " or \n, or escape values before writing:
• Change the condition to cover cancellation:

  if: failure() || cancelled()

• Align all default values to a single canonical type. The template (cx33) is the most authoritative reference for production intent — update Makefile.deploy and variables.tf defaults to match, or document the intended value explicitly in all locations.
• Add a jq presence check or ensure it is in the runner provisioning playbook:

  command -v jq > /dev/null || { echo "ERROR: jq not found on runner"; exit 1; }

  Alternatively, document jq as a runner requirement in the deploy infrastructure readme.
• Either (a) update .secret/secret.template.sh to use -iron_sdk / -iron_sdk.pub to match CI; or (b) if local dev intentionally uses different key files, add a comment in the template and in readme.md explaining the divergence.

[wanguardd]

System of Review completed analysis of 'Deployment | Add webhook notification'. Found 6 findings across 2 files.

📊 Progress Summary

Review #4

Changes since Review #3:

• 6 of 8 prior findings resolved — well-executed fixes.

Resolved this iteration:

• ✅ ORGANIZATIONAL: ## headers changed to ### throughout readme.md
• ✅ ORGANIZATIONAL: ### Scope section added
• ✅ SECURITY: Decoded GCP credentials now have private_key_id and client_email masked
• ✅ CORRECTNESS: Notify step condition changed to failure() || cancelled()
• ✅ DESIGN: jq presence check added (command -v jq)
• ✅ DESIGN: secret.template.sh SSH paths updated to -iron_sdk

Remaining: 1 RE-REQUEST (printf double-quote quoting, escalated to blocking) + 1 RE-REQUEST (suggestion). 4 new findings from this diff.

---

📋 Review Summary

Category	Count
🛡️ SECURITY	2
🐛 CORRECTNESS	1
📐 DESIGN	1
📖 DOCUMENTATION	2

---

🛡️ SECURITY

[🔴 Blocking | INTRODUCED] echo "::add-mask::${GCHAT_WEBHOOK_URL}" runs before the null guard in the Notify step. When GCHAT_WEBHOOK_URL is not configured, GitHub Actions injects an empty string. Masking an empty string registers "" as the secret pattern — the log sanitiser then replaces every empty-string match in all subsequent job output with ***, suppressing the Cleanup step's log entirely on any deployment failure where the webhook is not configured.

Evidence:

# deploy.yaml — Notify Google Chat on failure step
echo "::add-mask::${GCHAT_WEBHOOK_URL}"       # ← runs first, masks "" when var empty
if [ -z "${GCHAT_WEBHOOK_URL}" ]; then exit 0; fi
# Cleanup step runs after this — its shred output is suppressed when webhook not set

Action:

• Swap the two lines — null-guard first, then mask:

  if [ -z "${GCHAT_WEBHOOK_URL}" ]; then exit 0; fi
  echo "::add-mask::${GCHAT_WEBHOOK_URL}"

---

[🔴 Blocking | INTRODUCED — RE-REQUEST] All printf lines in the "Create secret.sh file" step embed the secret value inside literal double quotes: printf 'VAR="%s"\n' "${VAR}". If any secret value contains a " character — common in generated database passwords, SMTP connection strings, or API keys — the resulting line in .secret/-secret.sh becomes invalid shell. When Make's include parser reads the file, it fails with a syntax error and the deployment never starts, with no actionable log message pointing to the cause. This finding was raised in Review #3 and was not addressed.

Evidence:

# deploy.yaml — Create secret.sh file step (representative lines)
printf 'DATABASE_URL="%s"\n' "${DATABASE_URL}" >> .secret/-secret.sh
printf 'JWT_SECRET="%s"\n' "${JWT_SECRET}" >> .secret/-secret.sh
printf 'ALLOWED_ORIGINS="%s"\n' "${ALLOWED_ORIGINS}" >> .secret/-secret.sh

# If DATABASE_URL = 'postgresql://user:p@ss"word@host/db' (quote in password):
# Output line: DATABASE_URL="postgresql://user:p@ss"word@host/db"
#                                              ↑ unmatched quote — invalid shell
# make deploy fails silently at include time

Action:

• Add a pre-write guard that exits early if any value contains an embedded double-quote:

  for _val in "${DATABASE_URL}" "${JWT_SECRET}" "${ALLOWED_ORIGINS}" "${IC_TOKEN_SECRET}" "${IP_TOKEN_KEY}" "${IRON_SECRETS_MASTER_KEY}"; do
    if printf '%s' "${_val}" | grep -qF '"'; then
      echo "ERROR: Secret value contains embedded double-quote — update the secret to remove it"
      exit 1
    fi
  done

  Alternatively, write values without wrapping quotes and adjust the Makefile include parser accordingly.

---

🐛 CORRECTNESS

[🟡 Suggestion | EXISTING — RE-REQUEST] HOST_SERVER_TYPE has different default values across the deployment infrastructure. When vars.HOST_SERVER_TYPE is not set in the repository, the effective fallback depends on which file provides it — and the two infrastructure files disagree with the template. This finding was raised in Review #3 and was not addressed.

Evidence:

# deploy/Makefile.deploy
HOST_SERVER_TYPE := $(or $(call strip_quotes,$(HOST_SERVER_TYPE)),"cx23")

# deploy/hetzner_server_create/variables.tf
default = "cx23"

# .secret/secret.template.sh line 52
HOST_SERVER_TYPE="cx33"

# Result: infrastructure silently provisions cx23 when template implies cx33

Action:

• Align all three references to a single canonical default. If cx33 is the production intent (as the template documents), update Makefile.deploy and variables.tf. If cx23 is correct, update secret.template.sh.

---

📐 DESIGN

[🟡 Suggestion | INTRODUCED] SERVER_PORT, IRON_DEPLOYMENT_MODE, and ENABLE_DEMO_SEED are sourced from secrets.* and masked with ::add-mask::. A TCP port number, a runtime mode string (pilot/development/production), and a boolean flag are configuration values — not cryptographic material. Storing them as GitHub Secrets prevents administrators from seeing their current values in the UI and causes masking to redact every occurrence of 8080, production, or true from all workflow log lines.

Evidence:

# deploy.yaml — Create secret.sh file step (env block)
SERVER_PORT: ${{ secrets.SERVER_PORT }}
IRON_DEPLOYMENT_MODE: ${{ secrets.IRON_DEPLOYMENT_MODE }}
ENABLE_DEMO_SEED: ${{ secrets.ENABLE_DEMO_SEED }}

# deploy.yaml — masking block
echo "::add-mask::${SERVER_PORT}"           # "8080" redacted from all log lines
echo "::add-mask::${IRON_DEPLOYMENT_MODE}"  # "production" redacted from all log lines
echo "::add-mask::${ENABLE_DEMO_SEED}"      # "true" redacted from all log lines

Action:

• Move SERVER_PORT, IRON_DEPLOYMENT_MODE, and ENABLE_DEMO_SEED from secrets.* to vars.* in the workflow env: block. Remove the corresponding ::add-mask:: calls. Update readme.md to move these three entries from Required secrets to Required vars.

---

📖 DOCUMENTATION

[🟡 Suggestion | INTRODUCED] The Responsibility Table in readme.md includes a row registering readme.md itself. A readme describes the other files in the directory — not itself. The self-referential entry provides no navigation value and creates a circular reference.

Evidence:

# .github/workflows/readme.md — Responsibility Table
| `deploy.yaml`                       | Deploy to Hetzner on push to master or manual trigger |
| `deploy-check.yml`                  | Validate deploy infrastructure on PRs to master |
| `iron_token_manager_validation.yml` | Run iron_token_manager tests and schema checks |
| `readme.md`                         | Document workflow directory structure and usage |  ← self-referential

Action:

• Remove the readme.md row from the Responsibility Table.

---

[🟡 Suggestion | INTRODUCED] readme.md opens with "CI/CD pipeline for Iron Cage SDK." The canonical project name used throughout the workspace is "Iron Runtime" (workspace identifier: iron_runtime). "Iron Cage SDK" refers to the Python client SDK module (iron_sdk), not the server-side runtime this workflow deploys.

Evidence:

# .github/workflows/readme.md — line 3
CI/CD pipeline for Iron Cage SDK.
#                  ^^^^^^^^^^^^^^ — should be "Iron Runtime"

Action:

• Replace "Iron Cage SDK" with "Iron Runtime" in the readme opening paragraph.

---

Strong iteration — the structural fixes to readme.md and the deployment correctness improvements are well-executed. Two blocking items remain before merge.

[MykolaMS9]

Done in commit
chore (workflow): Incorporate the feedback from the review n.2
751beb7

Action:

• Swap the two lines — null-guard first, then mask:

  if [ -z "${GCHAT_WEBHOOK_URL}" ]; then exit 0; fi
  echo "::add-mask::${GCHAT_WEBHOOK_URL}"

• Add a pre-write guard that exits early if any value contains an embedded double-quote:

  for _val in "${DATABASE_URL}" "${JWT_SECRET}" "${ALLOWED_ORIGINS}" "${IC_TOKEN_SECRET}" "${IP_TOKEN_KEY}" "${IRON_SECRETS_MASTER_KEY}"; do
    if printf '%s' "${_val}" | grep -qF '"'; then
      echo "ERROR: Secret value contains embedded double-quote — update the secret to remove it"
      exit 1
    fi
  done

  Alternatively, write values without wrapping quotes and adjust the Makefile include parser accordingly.
• Align all three references to a single canonical default. If cx33 is the production intent (as the template documents), update Makefile.deploy and variables.tf. If cx23 is correct, update secret.template.sh.
• Move SERVER_PORT, IRON_DEPLOYMENT_MODE, and ENABLE_DEMO_SEED from secrets.* to vars.* in the workflow env: block. Remove the corresponding ::add-mask:: calls. Update readme.md to move these three entries from Required secrets to Required vars.
• Remove the readme.md row from the Responsibility Table.
• Replace "Iron Cage SDK" with "Iron Runtime" in the readme opening paragraph.

[wanguardd]

📊 Progress Summary

Review #5

Progress since Review #4:

• 5 of 6 prior findings resolved — strong iteration.

Resolved this iteration:

• ✅ SECURITY: GCHAT_WEBHOOK_URL null guard now executes before ::add-mask:: — ordering corrected
• ✅ DESIGN: SERVER_PORT, IRON_DEPLOYMENT_MODE, ENABLE_DEMO_SEED moved from secrets.* to vars.*
• ✅ CORRECTNESS: HOST_SERVER_TYPE default aligned to cx23 across Makefile.deploy, variables.tf, and secret.template.sh
• ✅ DOCUMENTATION: Self-referential readme.md row removed from Responsibility Table
• ✅ DOCUMENTATION: "Iron Cage SDK" replaced with "Iron Runtime" throughout readme.md

Remaining: 1 RE-REQUEST (printf guard — third consecutive ask). 1 new finding.

---

System of Review completed analysis of 'Deployment | Add webhook notification'. Found 2 findings across 1 file.

📋 Review Summary

Category	Count
🛡️ SECURITY	1
📐 DESIGN	1

---

🛡️ SECURITY

[🔴 Blocking | EXISTING — RE-REQUEST #3] The "Create secret.sh file" step still has no runtime guard against embedded double-quotes in secret values. Lines 97-101 now have a comment explaining the constraint, but comments are not enforcement. Twelve secret values are written with printf 'VAR="%s"\n' "${VAR}" — if any secret contains a literal " character (common in auto-generated database passwords, SMTP connection strings, or bcrypt-hashed tokens), the resulting line in .secret/-secret.sh becomes invalid shell, Make's include parser silently fails to parse the file, and the deployment never starts. The CI step exits 0 (Make exiting non-zero is surfaced only as a deploy failure), so there is no actionable error pointing to the double-quote as the root cause. This finding was first raised in Review #3, escalated to [🔴 Blocking] in Review #4, and remains open for the third consecutive review.

Evidence:

# deploy.yaml lines 97-101 — comment only, no runtime enforcement
# Secret values written below must NOT contain a literal double-quote (") character.
# Each value is wrapped in double quotes (VAR="<value>") so that the generated
# -secret.sh is valid shell and can be parsed by Make's include directive.
# An embedded " breaks the quoting and causes a silent parse failure at deploy time.
# If a secret must contain a double-quote, re-generate it without that character.

# deploy.yaml lines 120-125 — no guard before these writes
printf 'DATABASE_URL="%s"\n'            "${DATABASE_URL}"            >> .secret/-secret.sh
printf 'JWT_SECRET="%s"\n'              "${JWT_SECRET}"              >> .secret/-secret.sh
printf 'IC_TOKEN_SECRET="%s"\n'         "${IC_TOKEN_SECRET}"         >> .secret/-secret.sh
# ... 9 more printf lines with the same pattern

# If DATABASE_URL = 'postgresql://user:p@ss"word@host/db':
# Output: DATABASE_URL="postgresql://user:p@ss"word@host/db"
#                                          ↑ unmatched quote — Make parse error, no deploy

Action:

• Add a pre-write validation block immediately before the install call on line 102, after the ::add-mask:: block:

  for _val in "${DATABASE_URL}" "${JWT_SECRET}" "${IC_TOKEN_SECRET}" "${IP_TOKEN_KEY}" \
              "${IRON_SECRETS_MASTER_KEY}" "${ALLOWED_ORIGINS}" \
              "${GOOGLE_APPLICATION_PROJECT_ID}" "${GOOGLE_APPLICATION_REGION}" \
              "${GOOGLE_ENCRYPTION_KEY}" "${HOST_SERVER_NAME}" "${HOST_SERVER_IP}" \
              "${HETZNER_CLOUD_TOKEN}"; do
    if printf '%s' "${_val}" | grep -qF '"'; then
      echo "ERROR: Secret value contains embedded double-quote — update the secret to remove it before deploying"
      exit 1
    fi
  done

  This fails fast with an actionable message instead of writing a syntactically broken file to disk.

---

📐 DESIGN

[🟡 Suggestion | INTRODUCED] GOOGLE_APPLICATION_REGION is sourced from secrets.GOOGLE_APPLICATION_REGION (line 62) and masked with ::add-mask:: (line 86). A GCP region identifier is not sensitive information — region codes like europe-central2 appear in GCP billing dashboards, infrastructure documentation, and any public GCP resource URL. Masking it causes every occurrence of the region string to be replaced with *** in all runner log output, reducing diagnostic clarity of infrastructure logs without providing a security benefit. The project's own secret.template.sh shows the region as a hardcoded non-placeholder value (europe-central2 on line 10), confirming it is not treated as sensitive in local development. The three vars already moved to vars.* in this PR (SERVER_PORT, IRON_DEPLOYMENT_MODE, ENABLE_DEMO_SEED) demonstrate the author understands the correct distinction.

Evidence:

# deploy.yaml line 62 — sourced as a secret
GOOGLE_APPLICATION_REGION: ${{ secrets.GOOGLE_APPLICATION_REGION }}
# deploy.yaml line 86 — masked, suppresses region string from all log lines
echo "::add-mask::${GOOGLE_APPLICATION_REGION}"

# .secret/secret.template.sh line 10 — hardcoded value, not a placeholder
GOOGLE_APPLICATION_REGION="europe-central2"
# Compare: GOOGLE_ENCRYPTION_KEY and HETZNER_CLOUD_TOKEN are correctly placeholders

Action:

• Move GOOGLE_APPLICATION_REGION from secrets.GOOGLE_APPLICATION_REGION to vars.GOOGLE_APPLICATION_REGION in the env: block (line 62), remove its ::add-mask:: call (line 86), and update readme.md lines 43-45 to move it from Required secrets to Required vars.

---

Good momentum — five of six prior findings addressed cleanly in two commits. One more iteration on the runtime guard will bring this to merge-ready state.

---

❗ > Note: This System of Review operates with the same information access and constraints as any human developer working on this repository. While the system is highly accurate (99%+ reliability), any confusion or apparent misunderstanding typically indicates gaps in repository documentation, unclear code organization, or insufficient project discipline — the same issues that would impact any team member. If the System cannot locate critical information and draws incorrect conclusions, a human developer would face identical challenges. We maintain high standards to respect our teammates' time, our clients' investment, and the integrity of this project. Please ensure the repository provides clear, discoverable context for all reviewers.

Review approval is a quality floor, not a quality ceiling. It confirms that identified issues are resolved — not that the code is free of all issues. The System reviews the changes in this PR, not the entire codebase. Proactive quality is a developer responsibility: unit test coverage, edge case analysis, and consistency with the surrounding codebase cannot be delegated to any review process. If the review surfaces one instance of a pattern, the developer is responsible for auditing all occurrences of that pattern throughout the codebase — not just the specific line cited.

When addressing review feedback: Open a separate commit for each point you address, clearly referencing the finding in the commit message. This ensures knowledge is captured in the repository history and helps other developers avoid the same pitfalls. If you cannot address a point, leave a detailed comment in this PR review thread explaining specifically what is wrong with the finding or why it cannot be addressed — never ignore feedback silently. Use comments only in rare cases when the System has genuinely missed existing context in the repository; prefer commits as the primary response mechanism to build institutional knowledge.

[MykolaMS9]

Review

Add a pre-write validation block immediately before the install call on line 102, after the ::add-mask:: block:
Move GOOGLE_APPLICATION_REGION from secrets.GOOGLE_APPLICATION_REGION to vars.GOOGLE_APPLICATION_REGION in the env: block (line 62), remove its ::add-mask:: call (line 86), and update readme.md lines 43-45 to move it from Required secrets to Required vars.

Answer

    Design notes (not action items) ---
    [1] No runtime double-quote guard in secret values.
         A loop checking each secret value for an embedded " character has been
         deliberately omitted. All secrets stored in this repository are generated
         by deterministic tools (openssl rand, Hetzner API tokens, GCP service-account
         JSON fields extracted by jq) — none of which produce a double-quote in their
         output. Adding a grep-based guard would add noise and a failure mode for a
         class of input that cannot occur with the tooling documented in
         .secret/secret.template.sh. The constraint is documented in the comment below
         as a reminder for anyone who introduces a new secret source in the future.
         Secret values written below must NOT contain a literal double-quote (")
         character. Each value is wrapped in double quotes (VAR="<value>") so that
         the generated -secret.sh is valid shell parseable by Make's include directive.
    
     [2] GOOGLE_APPLICATION_REGION is kept in secrets.*, not vars.*.
         Although a GCP region string (e.g. europe-central2) is not cryptographic
         material, keeping it in secrets avoids exposing infrastructure topology in
         the repository's publicly visible Variables tab. The masking side-effect on
         log lines is acceptable: the region string appears only in GCP SDK output,
         not in human-readable progress messages, so diagnostic clarity is unaffected.
         Moving it to vars would require a separate secrets→vars migration step with
         no security or operational benefit to justify the churn.

[wanguardd]

📊 Progress Summary

Review #6

Progress since Review #5:

• Review #5 findings: 2 open (1 blocking, 1 suggestion)
• Both closed this iteration via author design explanations (see §Responses below)
• 2 new findings this review: 1 new blocking, 1 new suggestion

What changed:

• ✅ RE-REQUEST #3 (double-quote guard): Author provided explicit design rationale — accepted with caveats documented below
• ✅ GOOGLE_APPLICATION_REGION suggestion: Author provided infrastructure topology privacy rationale — accepted

Remaining: 1 blocking (H4 headings in readme.md). 1 new suggestion.

---

System of Review completed analysis of 'Deployment | Add webhook notification'. Found 2 findings across 1 file.

📋 Review Summary

Category	Count
🏛️ ORGANIZATIONAL	1
📖 DOCUMENTATION	1

---

🏛️ ORGANIZATIONAL

[🔴 Blocking | INTRODUCED] .github/workflows/readme.md uses four H4 (####) headings. The project documentation rulebook (§ Structure: H1 to H3 Only, line 382) states explicitly: "All documentation must use H1→H3 hierarchy only. No H2 (##), no H4 (####) or deeper." H4 is listed under prohibited heading levels with the rationale "Too deep, hurts scannability." Previous review rounds enforced the same rule for H2 headings; the same prohibition applies to H4.

Evidence:

# .github/workflows/readme.md — heading audit
Line  1: # GitHub Actions Workflows          ← H1 ✓
Line  5: ### Scope                           ← H3 ✓
Line 13: ### Directory Structure             ← H3 ✓
Line 15: #### Responsibility Table           ← H4 ✗ FORBIDDEN
Line 23: ### Workflows                       ← H3 ✓
Line 25: #### `deploy.yaml` — Deploy CI      ← H4 ✗ FORBIDDEN
Line 51: #### `deploy-check.yml` — Deployment CI                    ← H4 ✗ FORBIDDEN
Line 65: #### `iron_token_manager_validation.yml` — Iron Token Manager Validation  ← H4 ✗ FORBIDDEN

Action:

• Change all four #### headings to ###. The flat three-level structure (# → ###) is what the rulebook mandates and is sufficient for this document's depth.

---

📖 DOCUMENTATION

[🟡 Suggestion | INTRODUCED] .github/workflows/readme.md contains two description inaccuracies in the deploy.yaml Steps list:

1. Line 38 — The secret-file assembly entry reads "(safe for special characters)". The printf pattern used in deploy.yaml wraps every value in literal double quotes (VAR="%s"\n), which means values containing " produce a syntactically invalid shell file that silently breaks make deploy. The comment block in deploy.yaml (lines 97–101) explicitly warns that " is forbidden. Describing the pattern as "safe for special characters" contradicts this constraint — an operator reading the readme would not know the " exception exists.

2. Line 40 — The notification entry reads "Notify Google Chat on failure". The actual step condition is if: failure() || cancelled() — it also fires when a human cancels the workflow run from the GitHub UI. The notification message hardcodes "❌ *Deploy failed*" for both cases, so a cancellation produces a misleading alert.

Evidence:

# deploy.yaml line 135
if: failure() || cancelled()      # fires on cancellation too

# deploy.yaml line 152 — message text regardless of failure vs cancellation
'{"text": ("❌ *Deploy failed*\n*Repo:* " + $repo + ...)}'

# readme.md line 40 — says "on failure" only
5. Notify Google Chat on failure via `jq` + `curl --fail` ...

Action:

• Line 38: Change "(safe for special characters)" to "(values must not contain \" — see deploy.yaml comment)" or similar to match the documented constraint.
• Line 40: Change "on failure" to "on failure or cancellation" and note that the message text currently reads "Deploy failed" in both cases — if distinguishing the two is desirable, pass ${{ github.event_name }} as an --arg to jq and adjust the message template.

---

💬 Responses to Your Feedback

You posted design notes explaining two deliberate decisions. Here are my responses:

Double-quote guard

You said: All secrets in this repository are generated by deterministic tools (openssl rand, Hetzner API tokens, GCP service-account fields extracted by jq) — none of which produce a " character. A runtime guard would add noise for an impossible class of input.

Response: The machine-generated secrets argument is accepted — openssl rand, GCP SA fields, and Hetzner tokens are all safe. However, DATABASE_URL is user-configured, not tool-generated. secret.template.sh shows postgresql://iron_user:REPLACE_ME_PASSWORD@localhost:5432/iron_db — an operator sets the password manually, and a password manager or DBA tool may generate passwords containing ". The comment on lines 97–101 documents the constraint correctly; that is the accepted minimum. If DATABASE_URL is also always machine-generated in your deployment workflow, adding a note to that effect in the comment (# Note: DATABASE_URL is always generated by <tool> — no manual passwords) would close the concern without a code guard.

GOOGLE_APPLICATION_REGION in secrets

You said: Keeping it in secrets avoids exposing infrastructure topology in the publicly visible Variables tab. Masking impact on log lines is acceptable; the region appears only in GCP SDK output, not in human-readable messages.

Response: Infrastructure topology privacy is a valid operational concern. Accepted — no action required.

GCP private_key masking

You said: Masking the full multi-line private_key value is not reliably supported by ::add-mask::; masking private_key_id (a single-line UUID-like identifier) is the pragmatic safe minimum.

Response: Technically correct. Command substitution with $(jq -r '.private_key' ...) produces a multi-line string in the shell, and GitHub Actions' masking pattern operates per log line — registering a multi-line value does not guarantee reliable suppression of each individual line. Masking private_key_id and client_email (both single-line fields) is the correct pragmatic choice. Accepted.

---

Good iteration — the design-decision explanations are well-reasoned and address the context. One heading-level fix and two one-line description corrections are all that stand between this and merge-ready.

[MykolaMS9]

Done in commit
chore (workflow): Incorporate the feedback from the review n.5
cc7b446

Action:

• Change all four #### headings to ###. The flat three-level structure (# → ###) is what the rulebook mandates and is sufficient for this document's depth.
• Line 38: Change "(safe for special characters)" to "(values must not contain \" — see deploy.yaml comment)" or similar to match the documented constraint.
• Line 40: Change "on failure" to "on failure or cancellation" and note that the message text currently reads "Deploy failed" in both cases — if distinguishing the two is desirable, pass ${{ github.event_name }} as an --arg to jq and adjust the message template.

[wanguardd]

📊 Progress Summary

Review #9

Progress since Review #6:

• Previous blocking: H4 headings (4 occurrences) → resolved ✅
• Previous suggestion: Two description inaccuracies in readme.md → resolved ✅
• This review: 1 non-blocking suggestion (no new blocking findings)

Overall the PR has made excellent progress through 8 iterations. All security, design, and organizational findings from previous reviews are resolved. Ready to approve with one minor documentation cleanup note.

---

System of Review completed analysis of 'Deployment | Add webhook notification'. Found 1 finding across 1 file.

📋 Review Summary

Category	Count
🔍 QUALITY	1

---

🔍 QUALITY

[🟡 Suggestion | INTRODUCED] .github/workflows/readme.md contains a ### Directory Structure heading (line 13) with no content between it and the following ### Responsibility Table heading. An empty section adds visual noise and implies the directory structure description is forthcoming — it won't be, since the Responsibility Table immediately follows. Readers scanning the document will briefly pause expecting content that isn't there.

Evidence:

# .github/workflows/readme.md — lines 13–16
### Directory Structure

### Responsibility Table

| File | Responsibility |

Action:

• Remove the ### Directory Structure heading. The Responsibility Table can follow the Scope section directly, or if a structural label is desired, rename it to ### Responsibility Table only (eliminating the orphaned heading).

---

[MykolaMS9]

Done in commit
chore (workflow): Remove unused header
7f898e8

Action:

• Remove the ### Directory Structure heading. The Responsibility Table can follow the Scope section directly, or if a structural label is desired, rename it to ### Responsibility Table only (eliminating the orphaned heading).