feat(expenses): admin_notes column and lambda S3 object permissions - #314
Merged
Conversation
Groundwork for the expense review flow, split out so it lands before the lambda code that depends on it. - Adds a nullable `admin_notes TEXT` to `expenditures` for the reviewing admin's note. Additive and forward-only. - Grants the shared lambda role `s3:PutObject`/`s3:GetObject` on the reports bucket. The role had no S3 permissions at all, so presigning a receipt upload would have failed AccessDenied -- a presigned URL carries the signer's permissions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
This PR contains a database migration
It will be applied to the production database automatically when this PR merges, before the new lambda code is deployed. Please confirm before requesting review:
|
- Auto-formatted .tf files with terraform fmt - Updated README.md with terraform-docs Co-authored-by: nourshoreibah <nourshoreibah@users.noreply.github.com>
This was referenced Aug 12, 2026
Contributor
|
Database Types Check Complete The database schema files were modified, but the regenerated TypeScript types are identical to the existing ones. No changes were needed and the type definitions are already up to date. |
1 similar comment
Contributor
|
Database Types Check Complete The database schema files were modified, but the regenerated TypeScript types are identical to the existing ones. No changes were needed and the type definitions are already up to date. |
`make new-migration` scaffolds a header that reads "CREATE INDEX CONCURRENTLY / VACUUM will not work here". The guard grepped the raw file, so that sentence matched and every scaffolded migration failed before it contained any SQL at all. This is the first new migration since the guard landed -- the only other file, the baseline, predates the template -- so nothing had tripped it yet. Comments are now blanked before the destructive/CONCURRENTLY/user-insert scans. Blanking rather than deleting the lines keeps grep -n line numbers pointing at the real line. The allow-destructive opt-out still reads the raw file, since that marker is itself a comment. Verified the guard still fails a real CREATE INDEX CONCURRENTLY and a real ALTER TABLE ... DROP COLUMN, and still passes the baseline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Terraform Plan 📖
|
Contributor
|
Database Types Check Complete The database schema files were modified, but the regenerated TypeScript types are identical to the existing ones. No changes were needed and the type definitions are already up to date. |
nourshoreibah
marked this pull request as ready for review
August 12, 2026 02:12
nourshoreibah
added a commit
that referenced
this pull request
Aug 12, 2026
Conflicts in infrastructure/aws/lambda.tf and the generated infrastructure/aws/README.md. main's #314 added aws_iam_role_policy.lambda_s3_objects with the same actions (s3:PutObject, s3:GetObject) on the same resource (reports_bucket) as the lambda_reports_bucket policy added here, so the two were functionally identical. Kept main's resource and dropped the duplicate, folding this branch's rationale (report generation was failing AccessDenied, and a presigned GET needs the signer to hold GetObject) into its comment. README.md is terraform-docs output: took main's copy and dropped the aws_s3_bucket_policy.reports_bucket_policy row, since this branch deletes that resource. CI regenerates this file regardless.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Groundwork for the expense approve/deny flow, split out so it lands before the lambda code that depends on it. Migrations run against production before the new lambda code deploys, so this ordering is the required one.
Migration
20260812012951_add_expenditure_admin_notes.sqladds a nullableadmin_notes TEXTtoexpenditures— the reviewing admin's note. Additive and forward-only; safe for the lambda code that is live right now, which simply ignores the column.shared/types/db-types.d.tsis regenerated to match, per the commit-them-together rule inapps/backend/db/README.md.No status enum change was needed: the baseline check constraint already permits
needs_more_info, which is the value the review UI writes.IAM
Important
The shared lambda role had no S3 permissions at all. A presigned URL carries the signer's permissions, so presigning a receipt upload would have failed
AccessDeniedat the browser's PUT.Adds
s3:PutObject/s3:GetObjectscoped to${aws_s3_bucket.reports_bucket.arn}/*.This overlaps in intent with #310, which adds the same two actions while making the bucket private. If #310 merges first this becomes a redundant second inline policy under a different name (
branch-lambda-s3-objects) and can be dropped in review — the permissions are identical, so there is no ordering hazard either way.Deliberately does not touch the bucket's public-access block or bucket policy — that is #310's change, and duplicating it here would conflict.
CI guard bug
migrations-guardfailed on this PR, and it turned out to be a bug in the guard rather than a problem with the migration.make new-migrationscaffolds a header containing the sentence:The guard grepped the raw file, so that sentence matched its own
CONCURRENTLYcheck and the migration failed before it contained any SQL at all. Every migration generated by the project's own tooling would have failed this check. Nothing had tripped it yet because this is the first new migration since the guard landed — the only other file,0000_baseline_schema.sql, predates the template.Fix: blank out SQL comments before the destructive /
CONCURRENTLY/ user-insert scans. Blanking rather than deleting the lines keepsgrep -nline numbers pointing at the real line. The-- allow-destructive:opt-out still reads the raw file, since that marker is itself a comment.Verified locally that the guard still catches genuine violations:
CREATE INDEX CONCURRENTLY ...in real SQLALTER TABLE ... DROP COLUMNunder the scaffold header0000_baseline_schema.sqlVerification
terraform fmt -checkclean; the terraform-docs job auto-committed the README row for the new IAM resource.terraform validateneeds provider download and was not run.Follow-up: the application PR is #315.
🤖 Generated with Claude Code