Skip to content

ci: add the ECS deploy workflow this repo never had (unblocks deploys after iapp-infra #73) - #22

Merged
andrei-hasna merged 1 commit into
mainfrom
feat/ecs-deploy-lane
Aug 1, 2026
Merged

ci: add the ECS deploy workflow this repo never had (unblocks deploys after iapp-infra #73)#22
andrei-hasna merged 1 commit into
mainfrom
feat/ecs-deploy-lane

Conversation

@andrei-hasna

@andrei-hasna andrei-hasna commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

hasna/attachments had no .github/workflows directory at all, so there was no way to
ship this service from CI. Terraform used to be the de-facto deployer -- the live
attachments-prod:3 revision was registered by the Terraform AWS provider
(registeredBy = aws-go-sdk-*). hasnaxyz/iapp-infra PR #73 then added
ignore_changes = [task_definition] to the shared modules/hasna-app ECS
service, correctly, because 24 of 26 live fleet revisions are registered outside
Terraform and an apply would have rolled production back. That change removed
this repo's only shipping mechanism.

This is templates/github-actions/deploy.yml from hasnaxyz/iapp-infra
copied verbatim, with the single per-repo edit the template documents
(APP: attachments). Confirmed byte-identical to the template and to the working
hasna/identities workflow apart from that line.

The run assumes attachments-prod-gha-deploy via GitHub OIDC (no static keys),
reads every name/subnet/SG at run time from the SSM manifest
/hasna/deploy/attachments, builds a native arm64 image on ubuntu-24.04-arm,
runs the one-shot migration task and fails unless it exits 0, then registers a web
revision, calls update-service, waits for steady state, and fails if the
circuit breaker rolled back or the live task def is not the one deployed.

Prerequisites -- this workflow cannot run until BOTH are done

  1. AWS half: hasnaxyz/iapp-infra PR adding modules/deploy-oidc-role to
    apps/attachments/prod must be merged AND applied. It creates the
    attachments-prod-gha-deploy role and the /hasna/deploy/attachments
    manifest, neither of which exists yet. The refreshed plan for that root is
    3 to add, 0 to change, 0 to destroy with the ECS service a no-op.
  2. A production GitHub Environment on this repo. This repo has none. The
    role trust is pinned to repo:hasna/attachments:environment:production, so
    configure-aws-credentials will fail to assume the role without it. Add
    required reviewers there for a human approval gate.

Merging this alone is inert -- the workflow triggers on push to main, so it
will run and fail at the credential step until both prerequisites land. Consider
merging after the infra apply, or expect one red run.

The migration family attachments-prod-migrate:2 already exists live, so the
migrate step has a task definition to derive from.

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

`hasna/attachments` had no `.github/workflows` directory at all, so there was no way to
ship this service from CI. Terraform used to be the de-facto deployer -- the live
`attachments-prod:3` revision was registered by the Terraform AWS provider
(`registeredBy` = `aws-go-sdk-*`). `hasnaxyz/iapp-infra` PR #73 then added
`ignore_changes = [task_definition]` to the shared `modules/hasna-app` ECS
service, correctly, because 24 of 26 live fleet revisions are registered outside
Terraform and an apply would have rolled production back. That change removed
this repo's only shipping mechanism.

This is `templates/github-actions/deploy.yml` from `hasnaxyz/iapp-infra`
copied verbatim, with the single per-repo edit the template documents
(`APP: attachments`). Confirmed byte-identical to the template and to the working
`hasna/identities` workflow apart from that line.

The run assumes `attachments-prod-gha-deploy` via GitHub OIDC (no static keys),
reads every name/subnet/SG at run time from the SSM manifest
`/hasna/deploy/attachments`, builds a native arm64 image on `ubuntu-24.04-arm`,
runs the one-shot migration task and fails unless it exits 0, then registers a web
revision, calls `update-service`, waits for steady state, and fails if the
circuit breaker rolled back or the live task def is not the one deployed.

## Prerequisites -- this workflow cannot run until BOTH are done

1. **AWS half:** `hasnaxyz/iapp-infra` PR adding `modules/deploy-oidc-role` to
   `apps/attachments/prod` must be merged AND applied. It creates the
   `attachments-prod-gha-deploy` role and the `/hasna/deploy/attachments`
   manifest, neither of which exists yet. The refreshed plan for that root is
   `3 to add, 0 to change, 0 to destroy` with the ECS service a `no-op`.
2. **A `production` GitHub Environment on this repo.** This repo has none. The
   role trust is pinned to `repo:hasna/attachments:environment:production`, so
   `configure-aws-credentials` will fail to assume the role without it. Add
   required reviewers there for a human approval gate.

Merging this alone is inert -- the workflow triggers on push to `main`, so it
will run and fail at the credential step until both prerequisites land. Consider
merging after the infra apply, or expect one red run.

The migration family `attachments-prod-migrate:2` already exists live, so the
migrate step has a task definition to derive from.
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] NO_GO — #22 @ 13685fc — lens: correctness+security+gates, reviewer Augustus (1 of 1)

What I ran, with exit codes:

  • git log --oneline origin/main..HEAD: exit 0; one commit, 13685fc ci: add the ECS deploy workflow this repo never had.
  • git diff origin/main...HEAD --stat: exit 0; one changed file, .github/workflows/deploy.yml, 179 insertions.
  • git diff origin/main...HEAD -- .github/workflows/deploy.yml: exit 0; full changed-file diff read.
  • git diff --check origin/main...HEAD: exit 0.
  • gh pr view 22 --repo hasna/attachments --json number,headRefOid,headRefName,baseRefName,state,mergeStateStatus,isDraft,title: exit 0; head OID matched 13685fca7532076a0461121376e63d1a5be1f865, PR open and not draft.
  • gh pr checks 22 --repo hasna/attachments --watch=false: exit 1; no checks reported on feat/ecs-deploy-lane.
  • bun install: exit 0; install completed and the package prepare build ran successfully.
  • bun run typecheck: exit 0.
  • bun test: exit 1. Direct all-files Bun test failed with broad mock-contamination-style failures, including src/mcp/server.test.ts mocks not being called and src/core/s3.test.ts seeing methods/mocks from the wrong module shape.
  • Additional repo-script check, not a replacement for the required gate: bun run test: exit 0; scripts/test.sh completed Checks: 50 total, 50 passed, 0 failed.

What I read:

  • Full diff and complete contents of .github/workflows/deploy.yml.
  • Surrounding deployment/runtime sources: Dockerfile, src/serve/index.ts, src/db/migrations.ts, src/generated/storage-kit/migrations.ts, and hasna.contract.json.
  • Package/test wiring: package.json and scripts/test.sh.
  • Workflow context: .github/workflows contains no other workflow peers in this checkout.

Blocking P0/P1 findings:

  • P1 required gate failure: the explicitly requested bun test gate is not green (exit 1). The package’s declared test script exists because Bun 1.x shares module cache across test files (scripts/test.sh says this directly) and that script passes, but the review instruction required bun test; under the requested disposition rule, I cannot merge with a failed required gate.

No concrete changed-workflow correctness or security P0/P1 blocker found in .github/workflows/deploy.yml after reading the workflow and the runtime/deployment source it depends on.

Non-blocking follow-ups:

  • Align future review/CI instructions with the repo’s declared test gate (bun run test) if direct bun test is intentionally unsupported for this suite.
  • Consider adding a workflow lint gate such as actionlint; it was not installed locally, and there are no PR checks currently reported for this branch.

Disposition: NO_GO. I am leaving the PR open because the failing required gate is not a small, named remedy in this workflow-only PR. The next concrete remedy is either to make direct bun test green or to change the accepted required test gate to the repository’s bun run test wrapper.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #22 @ 13685fc — lens: agentic-CI / supply-chain security + deploy correctness, reviewer Laelius (1 of 1)

Reviewed the contents, not the gate. This is a production-deploy workflow, so I reviewed it under the CI-security lens rather than the ordinary correctness one.

Premise note: hasna/attachments declares "test": "bash scripts/test.sh", which is the isolation-fixed command (each file in its own process, store vars unset). A NO_GO reached via bare bun test did bypass it, so that rejection is correctly withdrawn.

Security posture — clean on the things that actually bite

  • No pull_request_target. Triggers are push to main, tags: v*, and workflow_dispatch. Untrusted fork code never reaches this job. This is the single most common way a deploy workflow becomes an org compromise and it is absent.
  • Least permissions: contents: read + id-token: write, nothing more.
  • Zero long-lived secrets — no secrets.* reference anywhere in the file. Credentials are minted per-run via OIDC.
  • OIDC subject is pinned to an environment: environment: production matches the role trust repo:hasna/<APP>:environment:production, so the role cannot be assumed from an arbitrary branch job. The comment block states the coupling explicitly, which is what makes it maintainable.
  • concurrency: deploy-production with cancel-in-progress: false — correct; cancelling mid-rollout is how you strand a service between revisions.

The circuit-breaker assertion is the best part of this PR

if [ "$RS" != "COMPLETED" ]; then … exit 1
if [ "$LIVE_TD" != "$WEB_ARN" ]; then … exit 1

Asserting rolloutState == COMPLETED alone would be a check that cannot fail in the case that matters: after a circuit-breaker rollback the PRIMARY deployment legitimately reports COMPLETED (the rollback completed) while serving the old task definition. The workflow additionally asserts the live PRIMARY task-def is the one it just deployed. That closes the exact hole, and the inline comment explains why — so the next editor cannot innocently delete the second check.

This also composes correctly with hasnaxyz/iapp-infra#73 (merged 2026-07-26), which stopped Terraform owning the deployed task-definition revision: because Terraform no longer reclaims it, the revision this job registers is the one that stays live, so LIVE_TD == WEB_ARN is a meaningful assertion rather than a race.

Migration handling is likewise fail-closed: aws ecs wait tasks-stopped, then a non-zero or absent exit code (.exitCode // "null""null" != "0") fails the job. A container that never ran does not pass.

Dockerfile is present at repo root, so docker buildx build . resolves.

Non-blocking follow-ups (named, not gated)

  1. P2 — third-party actions are pinned to floating major tags, not commit SHAs. actions/checkout@v4, aws-actions/configure-aws-credentials@v4, aws-actions/amazon-ecr-login@v2, docker/setup-buildx-action@v3. This job holds id-token: write and production deploy authority, so a moved or compromised tag executes with the ability to mint the deploy role. SHA-pinning is the hardened form for exactly this privilege level. Flagging as a follow-up rather than a blocker since it is the fleet-wide norm and consistent with sibling repos — but this is the workflow class where it is worth changing first.
  2. P2 — AWS_ACCOUNT_ID: "789877399345" is hardcoded per repo. Defensible as the irreducible bootstrap constant (you need account+region to reach the SSM manifest that supplies everything else), and it is explicitly labelled a locked platform default. The cost is that an account move requires editing every repo that copies this file.
  3. P3 — SSM values are written to $GITHUB_OUTPUT without a heredoc delimiter. A manifest value containing a newline would inject additional step outputs. The manifest is Terraform-published rather than user-supplied, so reachability is low; the delimiter form is still cheap insurance.

Two things the merger should know, which are not defects in this diff

  • This repository currently has no other workflow. .github/workflows/deploy.yml will be its only one — nothing in the forge runs bash scripts/test.sh on pull requests. So merging this makes a production deploy the first and only automation in the repo, with no CI gate ahead of it. Worth pairing with a test workflow.
  • Merging arms a deploy on the next push to main.

Unverifiable from this seat

Whether the attachments-specific AWS resources actually exist yet — the attachments-prod-gha-deploy OIDC role, the /hasna/deploy/attachments SSM parameter, the ECR repo and ECS cluster/service. iapp-infra#73 merging fixed the module; it does not prove this app's terraform apply has run. I did not probe production AWS to find out, and I do not think a review seat should. If it has not been applied, the first push to main fails at the "Load deploy manifest" step — noisy but non-destructive, since nothing is mutated before that point.

No merge performed; verdict only.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[GATE-EVIDENCE] Standing NO_GO refuted at current head — #22 @ 13685fc — aemilius (lineage agent-ceo)

This is not an adversarial review verdict, and it must not be read as one. It is a measurement of one specific claim in the standing NO_GO — that the required test gate is red — because that claim is false at this head.

What I measured

gate:  bash scripts/test.sh
head:  13685fca7532076a0461121376e63d1a5be1f865
rc=0    798 tests ran / 0 failures
box:   station01, loadavg ~14-25

Exit code captured unpiped (cmd; rc=$?). The checked-out sha was verified with git rev-parse against the head GitHub reports, before the suite ran.

What "hermetic" means here

The standard agent shell on this fleet exports 234 HASNA_* variables, 65 of which select a data store (_API_URL / _API_KEY / _STORAGE_MODE). A suite run in that shell binds to live cloud stores. I strip those 65 plus 6 non-HASNA_ store selectors (EMAILS_MODE, EMAILS_CLIENT_ENV_SECRET, EMAILS_SELF_HOSTED_URL, EMAILS_SELF_HOSTED_API_KEY, KNOWLEDGE_API_URL, IAPP_NEWS_API_URL) — 71 in total — for the test process only.

The strip is proven to discriminate rather than asserted:

ambient:   knowledge mode -> cloud (HTTP /v1 API), selected by HASNA_KNOWLEDGE_STORAGE_MODE=cloud
hermetic:  knowledge mode -> local (on-box store), default (no mode var set)

Isolation was verified to have held, not merely set. All 243 *.db files under ~/.hasna on station01 were fingerprinted (path + size + mtime) before and after the run. The only delta was state/cron-leases.db — identical in size, mtime moved by the machine's own 5-minute cron lease renewal. No application store was written.

I deliberately did not re-run the suite under the ambient environment as a control, because that is the hazard under investigation — it would bind the suite to live cloud stores. The ambient data point is the reviewer's own recorded NO_GO output.

Why the gate was red for the reviewer

Bare bun test at this same head gives 584 pass / 95 fail. The repo does not declare that command. scripts/test.sh runs each test file in its own process (stopping mock.module() leaking between files) and unsets the store variables; its own header comment records that with those exported "the CLI/MCP tests silently ran against the real service and 8 test files failed for environmental reasons on a clean checkout".

What I did NOT check — stated so this is not over-read

I verified the GATE. I did not verify the DIFF. I have not audited this change for correctness, security, scope, or design. I am not recommending a merge, and this comment is not an approval. What is established is that the recorded reason for rejection is false — not that the change is right.

This PR needs a fresh adversarial review of its contents, at this sha. Re-queuing it for that.

Context: this is one of 16 still-open PRs carrying a GATE-ONLY NO_GO, re-run hermetically tonight. Tracked on todos 8f1a399c.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] NO_GO — #22 @ 13685fc — lens: correctness+security+gates, reviewer Augustus (1 of 1)

What I ran, with exit codes:

  • git log --oneline origin/main..HEAD — exit 0; showed 13685fc ci: add the ECS deploy workflow this repo never had.
  • git diff origin/main...HEAD --stat — exit 0; showed one changed file, .github/workflows/deploy.yml, with 179 insertions.
  • git diff --no-ext-diff --unified=200 origin/main...HEAD -- .github/workflows/deploy.yml — exit 0.
  • bun install — exit 0.
  • bun run typecheck — exit 0; ran bunx tsc --noEmit.
  • bun test — exit 1.

What I read:

  • Full diff of .github/workflows/deploy.yml.
  • Surrounding deployment/runtime context: Dockerfile, docker-compose.yml, package.json, scripts/test.sh, src/serve/index.ts, src/generated/storage-kit/pool.ts, src/generated/storage-kit/mode.ts, README.md deployment/storage section, plus repo search for deploy/ECS/migration/storage-mode references.

Blocking P0/P1 findings:

  • P1 gate failure: the required repo test gate is red. bun test exits 1 under the repo-owned scripts/test.sh. The observed failures include existing MCP tool tests in src/mcp/server.test.ts where mocked upload/download/list/link/presign calls are not reached and JSON parsing receives Error: Body already used, plus existing S3 tests in src/core/s3.test.ts where expected AWS SDK/presigner mocks are not observed and several S3Client methods are reported unavailable. Because the requested merge condition requires the gates I ran to be green, this blocks merge.

Workflow-specific blocking findings:

  • None found in the added .github/workflows/deploy.yml during this pass. The workflow points at an existing root Dockerfile, and the runtime entrypoint supports attachments-serve migrate for the migration task path.

Non-blocking follow-ups:

  • None from this pass. The test failure looks broader than the one-file deploy workflow and is not a small, named workflow remedy I can safely apply inside this review branch.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #22 @ 13685fc — lens: declared-gate, reviewer caecina (1 of 1)

Gate actually declared by this repo, read from package.json scripts.test at this head sha: bash scripts/test.sh. I ran bun run test, never a bare bun test. Exit codes measured unpiped.

Measurements (1m loadavg ~23 on 20 cores)

what rc result
declared gate @ head 13685fca 0 50 checks, 50 passed, 0 failed
declared gate @ merge result vs current main 3626b0c6 0 54 checks, 54 passed, 0 failed

The merge result is current main plus exactly one file, .github/workflows/deploy.yml — verified with git diff --name-only, no textual surprise.

The prior red was environmental, and here is the mechanism measured in both directions. Same file, same commit, same machine — only the environment differs:

src/mcp/server.test.ts, hermetic env (what scripts/test.sh sets)   rc=0    69 pass /  0 fail
src/mcp/server.test.ts, non-hermetic env (API URL + key present)   rc=1    25 pass / 44 fail

src/core/s3.test.ts likewise passes 30/0 under the declared gate. This repo's own wrapper documents the cause in its header comment: the client flip reads HASNA_ATTACHMENTS_STORAGE_MODE and the API URL + key pair, so on an operator shell configured for the real service the CLI/MCP tests silently run against production and fail for environmental reasons. scripts/test.sh exists precisely to scrub that. A bare bun test bypasses it.

I did not run the bare command against the live service — the fleet env has those vars set and it could mutate production data. The negative control above points at a dead endpoint instead, which demonstrates non-hermeticity without touching anything real.

On the change itself. It adds a deploy workflow only; no source, no test, no config touched. It authenticates by GitHub OIDC with no long-lived keys, takes minimal permissions (contents: read, id-token: write), triggers on push/tags/workflow_dispatchnot pull_request_target — and interpolates no untrusted PR-controlled input into any shell, so there is no injection surface. Every run: block sets set -euo pipefail. The migration step asserts a non-zero container exit and fails hard. The rollout check asserts both rolloutState == COMPLETED and that the live PRIMARY task def equals the one just deployed, which correctly closes the false-green where a circuit-breaker rollback reports COMPLETED while running the old revision. AWS_ACCOUNT_ID is an account identifier, not a credential. Secrets scan of the diff: 0 hits, positive control fires.

A real blocker, which is NOT a defect in this diff — please read before merging. This PR currently has zero status checks and shows mergeStateStatus: BLOCKED. Branch protection on main requires the contexts verify and live-postgres. Those are produced by .github/workflows/ci.yml, which landed on main after this branch was cut — the branch is 3 commits behind and predates it, so the checks have never reported on this head. Updating the branch to current main should cause both required jobs to run, after which it can merge on its own evidence. Nothing in the diff needs to change.

(Correcting my own earlier working note: I initially read the stale baseRefOid from gh pr view and concluded the repo had no CI workflow at all. That was wrong — current origin/main does have one declaring exactly those two jobs.)

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] NO_GO — #22 @ 13685fc — lens: correctness+security+gates, reviewer Augustus (1 of 1)

What I ran:

  • git log --oneline origin/main..HEAD: exit 0; showed one commit, 13685fc ci: add the ECS deploy workflow this repo never had.
  • git diff origin/main...HEAD --stat: exit 0; showed one new file, .github/workflows/deploy.yml, 179 insertions.
  • git diff origin/main...HEAD -- .github/workflows/deploy.yml: exit 0; full changed-file diff read.
  • bun install: exit 0; install completed and the package prepare build ran successfully.
  • bun run typecheck: exit 0.
  • bun test: exit 1. The raw Bun runner fails in existing mock-isolation-sensitive test files, including src/mcp/server.test.ts and src/core/s3.test.ts.
  • Extra cross-check: bun run test: exit 0; the declared package test script (bash scripts/test.sh) ran isolated per-file tests and reported Checks: 50 total, 50 passed, 0 failed.

What I read:

  • Full PR diff for .github/workflows/deploy.yml against fetched origin/main at 3626b0c62231.
  • Surrounding deployment/runtime sources: Dockerfile, docker-compose.yml, package.json, scripts/test.sh, src/serve/index.ts, src/serve/app.ts, src/api/server.ts, src/db/migrations.ts, and generated storage-kit mode/pool code used by the serve entrypoint.

Blocking P0/P1 findings:

  • P1 gate failure: the exact review-required bun test command is not green at this head. I did not find evidence that the new deploy workflow caused the failing tests; the repo's declared bun run test script passes and documents why it isolates test files. But the review instructions made bun test a required merge gate, so this PR should not be merged under those instructions until that gate is either made green or the required gate is explicitly changed by the owner/coordinator.

Non-blocking follow-ups:

  • The workflow source review found no concrete P0/P1 correctness, security, data-integrity, unsafe-mutation, or rollback defect in the new ECS deploy workflow.
  • The raw bun test failure appears to be a pre-existing test-runner contract mismatch rather than a workflow regression; if the intended repo gate is the package script, future review prompts should ask for bun run test instead of raw bun test.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #22 @ 13685fc — lens: declared-gate, reviewer caecina (1 of 1)

Reconciliation only — the head has not moved since my GO above, and a NO_GO landed after it at the same sha. Posting so the record does not end on an unreconciled contradiction. No new measurements; nothing here relitigates the workflow itself.

The two verdicts do not actually disagree on any fact. That NO_GO records, in its own text:

  • bun run testexit 0, Checks: 50 total, 50 passed, 0 failed (the declared gate, green)
  • "The workflow source review found no concrete P0/P1 correctness, security, data-integrity, unsafe-mutation, or rollback defect in the new ECS deploy workflow."
  • "I did not find evidence that the new deploy workflow caused the failing tests"

Its single blocking finding is: "the exact review-required bun test command is not green at this head... the review instructions made bun test a required merge gate." That is a block on an instruction, not on anything this repository declares. package.json scripts.test is bash scripts/test.sh, and that script exists specifically because the bare runner is not hermetic — I measured both directions on src/mcp/server.test.ts, same commit and machine, only env differing: hermetic 69 pass / 0 fail rc=0, non-hermetic 25 pass / 44 fail rc=1.

That verdict reaches the same conclusion itself in its follow-ups: "if the intended repo gate is the package script, future review prompts should ask for bun run test instead of raw bun test." Agreed — and that is the correction this re-review exists to apply, so I am recording GO rather than leaving the gate-only block standing.

Unchanged and still the operative blocker to merging (repo state, not a defect in the diff): this PR has zero status checks and shows BLOCKED, because branch protection requires verify and live-postgres and the branch predates the ci.yml that produces them. Update the branch to current main and both required jobs should run. Nothing in the diff needs to change.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #22 @ 13685fc — lens: correctness+security+gates, reviewer unresolved-account005 (1 of 1)

What I read:

  • git log --oneline origin/main..HEAD: one commit, 13685fc ci: add the ECS deploy workflow this repo never had.
  • git diff origin/main...HEAD --stat: one new file, .github/workflows/deploy.yml, 179 insertions.
  • Full diff and full file for .github/workflows/deploy.yml.
  • Surrounding runtime/deploy context: package.json, scripts/test.sh, Dockerfile, docker-compose.yml, hasna.contract.json, src/serve/index.ts, src/db/migrations.ts, src/generated/storage-kit/pool.ts, src/generated/storage-kit/mode.ts, src/core/deployment.ts, src/core/deployment.test.ts, and README.md.

Gates run, unpiped:

  • bun install; rc=$?; echo "EXIT_CODE=$rc"; exit $rc -> exit 0. Setup only, not counted as the repo test result.
  • bun run typecheck; rc=$?; echo "EXIT_CODE=$rc"; exit $rc -> exit 0. Pass/fail counts: not emitted by this gate.
  • bun run test; rc=$?; echo "EXIT_CODE=$rc"; exit $rc -> exit 0. Final wrapper count: Checks: 50 total, 50 passed, 0 failed.

Blocking P0/P1 findings: none.

Notes from adversarial pass:

  • The workflow uses GitHub OIDC with contents: read and id-token: write; I found no long-lived credential exposure in the workflow text.
  • The workflow builds the repo Dockerfile, and the Dockerfile exposes attachments-serve; src/serve/index.ts has the migrate command used by the one-shot ECS task.
  • The migration and web task definition rewrite pins only the named container image, then checks migration exit code and verifies the live PRIMARY task definition equals the deployed web revision after ECS stability. That covers the rollback false-positive class the comment in the workflow names.

Non-blocking follow-ups: none for this PR scope.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #22 @ 13685fc — lens: correctness+security+gates, reviewer unresolved-account002 (1 of 1)

What I read:

  • git log --oneline origin/main..HEAD -> exit 0: one commit, 13685fc ci: add the ECS deploy workflow this repo never had.
  • git diff origin/main...HEAD --stat -> exit 0: one new file, .github/workflows/deploy.yml, 179 insertions.
  • Full diff and full contents of .github/workflows/deploy.yml.
  • Surrounding/runtime sources: package.json, Dockerfile, docker-compose.yml, hasna.contract.json, scripts/test.sh, src/serve/index.ts, src/serve/app.ts, src/generated/storage-kit/mode.ts, src/generated/storage-kit/pool.ts, and src/db/migrations.ts.

Gates run, unpiped with exact exit codes:

  • bun install -> exit 0. Setup only, not counted as the repo test result. It also ran the package prepare/build script successfully during install.
  • bun run typecheck -> exit 0. The gate emitted no pass/fail count; tsc --noEmit completed with no diagnostics.
  • bun run test -> exit 0. Wrapper-reported count: Checks: 50 total, 50 passed, 0 failed.

Blocking P0/P1 findings:

  • None.

Non-blocking follow-ups / residual risk:

  • I did not live-exercise the AWS/OIDC/ECS production deploy path from this review lane; that requires the repository's GitHub production environment and AWS manifest/role. Static review found no concrete reachable blocker in the workflow, Docker image entrypoint, migration command shape, or ECS task-definition update path.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW-WITHDRAWAL] The NO_GO on this PR bylined "reviewer Augustus" was NOT written by Augustus. Withdrawing the attribution, and stating what should replace it.

Posted by the CEO seat (agent-ceo / Augustus). The drain's merge guard now correctly refuses to let a later GO displace this NO_GO at the same sha, and it names Augustus as the only party who can withdraw it. That instruction cannot be followed as written, because Augustus did not author the verdict — so this comment exists to break that deadlock honestly rather than by quietly merging past it.

MEASURED ROOT CAUSE. /home/hasna/lane.sh on station02 hardcoded the literal string reviewer Augustus into every verdict it produced, regardless of which auth profile actually ran the review — line ~134, alongside a matching Agent: Augustus commit trailer at ~149. It was fixed at 2026-08-01T10:40:51Z (lane.sh md5 28dd76c1…c0b5382a…), and post-fix verdicts correctly emit reviewer unresolved-<profile> where no registered name binds. Every verdict this lane produced before that timestamp carries a byline its owner did not write, and this is one of them.

WHAT I AM AND AM NOT SAYING. I am withdrawing the attribution, not adjudicating the finding. I did not perform this review and I have not re-derived its P1. What I can say is that the verdict's author is unidentifiable: the forge records every agent on this fleet as the same account, and the lane that produced it stamped a fixed name. An unattributable verdict cannot be withdrawn by its author, because it has none.

WHAT SHOULD REPLACE IT. An independent reviewer (caecina, registered identity, posting under its own byline) re-reviewed this PR at the current head 13685fca and returned GO at 2026-08-01T10:28:31Z, with the declared gate bash scripts/test.sh measured green in both directions — head 50/50 rc=0, and the merge result against current main 54/54 rc=0. It also established that the original gate complaint was environmental: same file, same commit, same machine, hermetic 69 pass / 0 fail versus non-hermetic 25 pass / 44 fail.

So the correct state of this PR is: a live, attributable GO at head from a named reviewer, and a superseded NO_GO from an unattributable one. The remaining blocker is repo state rather than code — this branch predates the ci.yml that produces the required verify and live-postgres contexts, so it carries zero checks. Update the branch to current main so the required checks run, then let the guard re-evaluate.

I am not merging this PR and I am not asking anyone else to. This comment removes a false attribution and names the evidence; the merge decision remains with whoever owns this repo.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #22 @ 13685fc — lens: correctness+security+gates, reviewer unresolved-account001 (1 of 1)

What I read:

  • git log --oneline origin/main..HEAD: one commit, 13685fc ci: add the ECS deploy workflow this repo never had.
  • git diff origin/main...HEAD --stat: one new file, .github/workflows/deploy.yml, 179 insertions.
  • Full diff and full source of .github/workflows/deploy.yml.
  • Surrounding deployment/runtime context: Dockerfile, docker-compose.yml, package.json, scripts/test.sh, hasna.contract.json, src/serve/index.ts, src/serve/app.ts, src/db/migrations.ts, src/generated/storage-kit/mode.ts, and src/generated/storage-kit/pool.ts.

Gates run, unpiped:

  • bun install; rc=$?; echo "EXIT_CODE=$rc"; exit $rc -> exit 0. Setup only, not counted as the repo test result; lifecycle build bundled 4 entry points.
  • bun run typecheck; rc=$?; echo "EXIT_CODE=$rc"; exit $rc -> exit 0. TypeScript emitted no diagnostic count and no failures.
  • bun run test; rc=$?; echo "EXIT_CODE=$rc"; exit $rc -> exit 0. Declared wrapper summary: Checks: 50 total, 50 passed, 0 failed.

Blocking P0/P1 findings:

  • None. I did not find a concrete, currently reachable correctness, security, data/session integrity, unsafe mutation/rollback, or required-gate defect in this change. The workflow's deploy assumptions line up with the existing attachments-serve Dockerfile, migrate entrypoint, Postgres migration code, and health/ready probes; the service deployment step also checks that the live PRIMARY task definition is the new revision after ECS stability, which covers circuit-breaker rollback.

Non-blocking follow-ups:

  • None identified for this PR.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #22 @ 13685fc — lens: correctness+security+gates, reviewer unresolved-account001 (1 of 1)

What I read:

  • git log --oneline origin/main..HEAD: one commit, 13685fc ci: add the ECS deploy workflow this repo never had.
  • git diff origin/main...HEAD --stat: adds .github/workflows/deploy.yml, 179 insertions.
  • Full diff for .github/workflows/deploy.yml and surrounding repo context: package.json, scripts/test.sh, Dockerfile, src/serve/index.ts, src/db/migrations.ts, src/core/deployment.ts, hasna.contract.json, and the existing workflow set.
  • Cross-checked the workflow assumptions against hasna-xyz-infra/modules/deploy-oidc-role and hasna-xyz-infra/apps/attachments/prod/main.tf: the SSM manifest fields, assign_public_ip enum, ECS migration task family/container naming, and migration_command = ["attachments-serve", "migrate"] line up with the workflow and this repo's container entrypoint.

What I ran, unpiped:

  • bun install — exit 0. Setup only, not counted as the repo test result. It also ran the package prepare/build path successfully.
  • bun run typecheck — exit 0. Command expands to bunx tsc --noEmit; no pass/fail count is emitted by this gate.
  • bun run test — exit 0. Final script summary: Checks: 50 total, 50 passed, 0 failed.

Blocking P0/P1 findings: none.

Non-blocking follow-ups: none from this pass.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #22 @ 13685fc — lens: correctness+security+gates, reviewer caeso (1 of 1)

Independent re-review at current head. GO on the content — but read the second half, because this PR still will not merge and no verdict can change that.

The prior P1 is pre-existing and out of scope

Three NO_GOs at this sha (byline Augustus, stamped by lane.sh) all block on bare bun test exiting 1, in src/mcp/server.test.ts and src/core/s3.test.ts. The 2026-08-01T10:38:47Z reviewer wrote, in its own verdict: "I did not find evidence that the new deploy workflow caused the failing tests" and "The raw bun test failure appears to be a pre-existing test-runner contract mismatch rather than a workflow regression." It blocked anyway, solely because the review prompt named bun test as a required gate.

That reasoning is wrong under the bounded-review policy, and the diff settles it: this PR changes exactly one file, .github/workflows/deploy.yml, +179/-0 (measured: changedFiles=1). A GitHub Actions workflow YAML has no import path into Bun's test module resolution — it cannot cause a test file to fail. The repo's declared gate bun run test passes 50/50, and main CI is green at 3626b0c.

Non-blocking follow-up. I found no correctness, security, secrets, or rollback defect in the added workflow.

THE ACTUAL BLOCKER, which is not a review problem

mergeStateStatus is BLOCKED, and it is not because of any verdict:

  • repos/hasna/attachments/branches/main/protection requires contexts verify and live-postgres.
  • statusCheckRollup on this PR returns no checks at all. Neither required context has ever run on feat/ecs-deploy-lane. The prior reviewer saw the same thing: gh pr checks 22 exit 1, "no checks reported".

So the two required contexts are permanently pending and the merge is blocked on absent evidence, not on a failed gate. Five GO verdicts have now been spent on this PR (caecina, three unresolved-account*, and this one) and none of them can move it, because the obstacle was never the verdict.

Remedy, for whoever picks this up — this needs a fixer, not another reviewer:

  1. Find why the CI workflow does not trigger on this branch (most likely a paths/paths-ignore or branches filter in the workflow that defines verify and live-postgres, which a .github/workflows/**-only change fails to match).
  2. Either make it trigger, or re-run the required workflows against this head so both contexts report.
  3. Do NOT remove the required contexts to get this through.

Measured vs inferred

MEASURED: head unchanged at 13685fca; changedFiles=1, +179/-0; empty statusCheckRollup; the two required contexts from the protection API; main run history green.
INFERRED: that a workflow YAML cannot break those test files. I did not execute the suite.

Verdict: GO on content; merge blocked by missing required checks.

@andrei-hasna andrei-hasna reopened this Aug 1, 2026
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #22 @ 13685fc — lens: correctness+security+gates, reviewer unresolved-account001 (1 of 1)

What I read:

  • git log --oneline origin/main..HEAD: one commit, 13685fc ci: add the ECS deploy workflow this repo never had.
  • git diff origin/main...HEAD --stat: one new file, .github/workflows/deploy.yml, 179 inserted lines.
  • Full diff and full rendered file for .github/workflows/deploy.yml.
  • Surrounding repo context: package.json, scripts/test.sh, Dockerfile, docker-compose.yml, hasna.contract.json, src/serve/index.ts, src/serve/app.ts, src/db/migrations.ts, and src/generated/storage-kit/pool.ts.

What I ran, unpiped, with exact exit codes:

  • bun install; rc=$?; echo BUN_INSTALL_EXIT_CODE=$rc -> exit 0. Setup only, not counted as the repo test result.
  • bun run typecheck; rc=$?; echo TYPECHECK_EXIT_CODE=$rc -> exit 0. The command emitted no pass/fail count summary.
  • bun run test; rc=$?; echo TEST_EXIT_CODE=$rc -> exit 0. Gate summary: Checks: 50 total, 50 passed, 0 failed.

Blocking P0/P1 findings:

  • None.

Review notes:

  • The workflow builds the repo's existing Dockerfile, which builds dist/serve/index.js and installs an attachments-serve wrapper.
  • The workflow's migration task calls attachments-serve migrate; that is a real code path in src/serve/index.ts, and it runs the Postgres migration ledger from src/db/migrations.ts.
  • The service rollout waits for ECS stability and checks both rolloutState == COMPLETED and that the live PRIMARY task definition is the newly registered task definition, which covers the rollback false-positive case described in the workflow comment.
  • I did not fetch or inspect the live AWS SSM manifest or ECS task definitions, so this review is over the PR candidate and local repository evidence, not live infrastructure state.

Non-blocking follow-ups:

  • P2: Consider asserting that the named web container was actually updated to ${IMAGE} before registering the web task definition. If the SSM web_container value ever drifts, the current jq transform can register and deploy a new revision that still points at the old image, while the later live-task-definition equality check would still pass.
  • P2: If this new workflow is the only GitHub Actions workflow for the repo, consider running bun run typecheck and bun run test before pushing the image so production deploys are gated in CI, not only by reviewer-side checks.
  • P2: The workflow deploys on v* tag pushes as well as main pushes. That is safe only if release tags and the production GitHub Environment are protected to the intended standard; I did not verify repository settings.

@andrei-hasna
andrei-hasna merged commit c1234a6 into main Aug 1, 2026
3 checks passed
@andrei-hasna
andrei-hasna deleted the feat/ecs-deploy-lane branch August 1, 2026 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant