[Enhancement] - Automate GCS bucket, datastore, and GCP app creation

[leekahung]

What type of PR is this? (check all applicable)

• Refactor
• Feature
• Bug Fix
• Optimization
• Documentation Update
• Infrastructure
• Maintenance

Description

This PR is a follow-up to #345, where bucket, datastore, and app creation can be made via automation scripts from the backend. The current flow is for file-based storage and deployment. Only users with GCP access can run this automated flow in CLI.

The creation process has been tested, see bucket for tenantfirstaid-05122026-test in GCS and the datastore for tenantfirstaid-test-fresh in AI Applications on GCP, and the app for tenantfirstaid-test-fresh in AI Applications on GCP.

Related Tickets & Documents

• Related Issue Automate GCS bucket creation and split metadata generator from ASCII encoder #347
• Closes Automate GCS bucket creation and split metadata generator from ASCII encoder #347

QA Instructions, Screenshots, Recordings

Please replace this line with instructions on how to test your changes, a note on the devices and browsers this has been tested on, as well as any relevant images for UI changes.

To test, you can run the following from the backend directory:

1. make generate-metadata GCS_BUCKET_NAME=<name-of-your-new-bucket>
2. make upload-to-gcs GCS_BUCKET_NAME=<name-of-your-new-bucket>
3. make create-datastore-gcs GCS_BUCKET_NAME=<name-of-your-new-bucket> DATASTORE_NAME=<name-of-your-new-datastore>
4. make create-app-gcs DATASTORE_ID=<name-of-your-new-datastore> APP_NAME=<name-of-your-new-app>

---

What each step does:

1. Creates the new metadata.jsonl reference (PR Include annotations for existing statutes and ASCII enforcement #345)
2. Creates a new bucket in GCS and uploads metadata.jsonl and the documents to the new bucket (this PR)
3. Creates a new datastore to and connects your bucket to the datastore (this PR)
4. Creates a new app on GCP for the datastore to connect to (this PR)

Added/updated tests?

• Yes
• No, and this is why: please replace this line with details on why tests have not been included
• I need help with writing tests

Documentation

• If this PR changes the system architecture, Architecture.md has been updated

[optional] Are there any post deployment tasks we need to perform?

[leekahung]

@claude pr-review

[claude]

Claude finished @leekahung's task in 2m 43s —— View job

---

PR Review

• Read CLAUDE.md and architecture docs
• Review changed files (Makefile, scripts, tests)
• Analyze code quality, security, performance
• Check tests coverage and quality
• Review documentation updates
• Post final review

---

Nice work, @leekahung. The split into four focused scripts (enforce_ascii → generate_metadata_jsonl → upload_to_gcs → create_datastore_gcs → create_app_gcs) reads well — each step has a clear contract, dry-run support, and proper error wrapping. Test coverage is good, with happy/error paths and dry-run all covered. Below are findings ordered by severity.

Findings

Higher impact

1. Bucket↔datastore location compatibility is not validated upfront — backend/scripts/create_datastore_gcs.py:99

   The bucket is created with --location defaulting to US (a multi-region) in upload_to_gcs.py, while the datastore defaults to --location global. If the user picks mismatched regions (e.g. LOCATION=eu on upload-to-gcs and LOCATION=us on create-datastore-gcs), the Vertex AI import will fail mid-pipeline rather than during planning. Consider either (a) a storage_client.get_bucket(bucket).location pre-flight in create_datastore_gcs.main before calling the API, or (b) at least documenting the constraint loudly near both --location flag helps. Fix this →

2. InvalidUtf8Error raise lacks from clause — backend/scripts/enforce_ascii.py:113-115

   except UnicodeDecodeError:
       raise InvalidUtf8Error(
           f"{path.name}: file is not valid UTF-8 ..."
       )

   This is a ruff B904 violation (raise-without-from-inside-except). Should be raise InvalidUtf8Error(...) from e or from None. Worth checking whether make lint currently flags it on this branch. Fix this →

3. Brittle test assertions on protobuf error text — backend/tests/test_upload_to_gcs.py:114, 123

   test_invalid_json_raises matches "Failed to load JSON" and test_unknown_field_raises matches 'no field named "unknown_field"'. Both strings are produced by google.protobuf.json_format internals and have historically changed between protobuf versions. Consider asserting on the file path + line number prefix (metadata_path.name:1: from upload_to_gcs._iter_documents) which is wording you control, plus a looser match on the protobuf body.

Lower impact / nits

4. MagicMock(name=...) does not set .name — backend/tests/test_create_datastore_gcs.py:46, test_create_app_gcs.py:37

   MagicMock(name=...) configures the mock's internal repr name, not its .name attribute. The tests that use this in _make_client only assert on the returned mock identity, so they pass — but it's a footgun. Use m = MagicMock(); m.name = "..." (as test_happy_path_creates_datastore_and_imports already does correctly).

5. No end-to-end test for --no-wait and --dry-run Makefile target flags in create_datastore_gcs.main — there's coverage for import_documents(wait=False) directly, but no end-to-end happy-path test that exercises the main() → wait=False flow. Optional.

6. SEARCH_TIER_STANDARD hardcoded — backend/scripts/create_app_gcs.py:55-57

   No flag for SEARCH_TIER_ENTERPRISE. Fine for now since you're standardizing on Standard, but worth a short comment ("SEARCH_TIER_STANDARD is sufficient for keyword + semantic; bump to Enterprise if/when we need advanced LLM features") so a future reader knows it was a deliberate choice.

7. upload_to_gcs.upload_files is serial — backend/scripts/upload_to_gcs.py:131-147

   ~12 docs is fine. If the corpus grows past a few dozen files, concurrent.futures.ThreadPoolExecutor with the storage client would speed it up meaningfully. Mention only — no action needed today.

8. enforce_ascii reads each file up to twice — backend/scripts/enforce_ascii.py:104-115

   Reads once as ASCII, falls back to a UTF-8 reread. For a few hundred small .txt files this is negligible, but path.read_bytes() + a single decode would be cleaner.

9. _collect_unrecognized returns {char: first_position} but the field is named like a count — backend/scripts/enforce_ascii.py:80-86

   The docstring is accurate, but a future reader skimming enforce_ascii may misread unrecognized: dict[str, int] as a count map. Minor; consider dict[str, int] → dict[str, int] # char -> first byte offset or renaming positions to make the intent obvious.

10. google-cloud-storage is in dev-only deps — backend/pyproject.toml:48

    Correct: the production code never imports google.cloud.storage, only the scripts do. Good call to scope this to dev. Worth confirming nothing in tenantfirstaid/ ever ends up needing it.

What I liked

• Clean separation of concerns across the four scripts — each is independently runnable and has its own --dry-run.
• Errors are wrapped in domain-specific UploadError/DatastoreError/AppError with actionable messages.
• The "fail fast locally before touching GCS" pattern in upload_to_gcs.main() (via validate_and_rewrite_tree(check_only=True)) is a nice safety net for files added between generate-metadata and upload-to-gcs.
• Console URLs printed at the end of create_datastore_gcs.py and create_app_gcs.py are a thoughtful UX touch.
• metadata.jsonl flat-URI invariant is enforced in _uri_to_filename — defensive and worth keeping.
• Architecture.md and CLAUDE.md updates are accurate; the IAM permission notes in CLAUDE.md will save someone an hour.
• ASCII enforcement extraction into its own module unblocks reuse and gets its own targeted tests — good refactor.

Summary

This is in good shape to merge once the from e chain (#2) is fixed. The bucket/datastore region check (#1) is the only one with real operational consequence; the rest are polish.
• Branch: issue-347/automate-gcs-bucket-creation-uploads

[leekahung]

@claude pr-reivew

[claude]

Claude finished @leekahung's task in 2m 53s —— View job

---

PR Review

• Read CLAUDE.md and architecture docs
• Review changed files (Makefile, scripts, tests)
• Analyze code quality, security, performance
• Check test coverage and quality
• Review documentation updates
• Post final review

---

Re-reviewing after the most recent push (ad3c9bb Adjust script logic; update unit tests). Looks like you addressed the biggest items from the previous review — flagging that explicitly below and then listing the new things I noticed.

Resolved since prior review

• Bucket↔datastore region check is now wired in — check_bucket_location_compat() runs before create_data_store() in create_datastore_gcs.main() (backend/scripts/create_datastore_gcs.py:221), with parametrized happy/incompatible coverage in test_create_datastore_gcs.py:48-87. The test_incompatible_bucket_location_raises_before_api_calls test also asserts that no datastore is created when the check fails — exactly the safety net needed.
• InvalidUtf8Error now uses from e — backend/scripts/enforce_ascii.py:106-109. B904 satisfied.
• Protobuf assertions made resilient — test_invalid_json_raises / test_unknown_field_raises now match the metadata.jsonl:1: prefix you control rather than the protobuf internals (backend/tests/test_upload_to_gcs.py:114,123).
• MagicMock(name=...) footgun avoided — bucket/datastore/app mocks consistently use m.name = "..." attribute assignment (test_create_datastore_gcs.py:44, test_create_app_gcs.py:38).

New / remaining findings

Higher impact

1. No client-side validation of --datastore-name / --app-name — backend/scripts/create_datastore_gcs.py:174, backend/scripts/create_app_gcs.py:87

   Both arg helps say "Lowercase letters, digits, hyphens (1-63 chars)" but the constraint isn't enforced before calling Discovery Engine. Invalid IDs surface as a generic InvalidArgument from the server. A 3-line re.fullmatch(r"[a-z0-9-]{1,63}", name) precheck would short-circuit with a clear error. Minor, but cheap. Fix this →

2. Partial-failure cleanup is undocumented — backend/scripts/create_datastore_gcs.py:228-244

   If create_datastore succeeds but import_documents fails (network, permissions, bad metadata.jsonl), the new datastore is left dangling. A re-run will fail with "already exists" until the user manually deletes it via the console. Two reasonable options: (a) wrap import_documents in a try/except that deletes the datastore on failure, or (b) just document this in CLAUDE.md so the user knows what to clean up. (a) is more user-friendly; (b) is fine if you're conservative about deletions. Fix this →

Lower impact / nits

3. bucket.location can be None for very new buckets in unusual states — backend/scripts/create_datastore_gcs.py:69

   The .upper() call assumes a string. In practice get_bucket() populates it, but a defensive bucket.location or "" (or a clearer "bucket has no location metadata" error) would avoid a confusing AttributeError. Edge case only.

4. metadata_blob content type — backend/scripts/upload_to_gcs.py:145

   application/jsonl works, but the more canonical media type for JSON Lines is application/x-ndjson. Vertex AI Search reads via input_uris so neither matters functionally — flag only if you want pedantic correctness.

5. enforce_ascii.py still reads each non-ASCII file twice in the UnicodeDecodeError path — backend/scripts/enforce_ascii.py:104-115

   Carried over from the prior review; still nbd for ~hundreds of small files. Leaving in the list for completeness.

6. SEARCH_TIER_STANDARD hardcoded — backend/scripts/create_app_gcs.py:55-57

   No flag and no comment explaining the choice. Same note as last time: a one-liner ("standard is sufficient for keyword + semantic; enterprise unlocks advanced LLM features") would help a future reader.

7. Reentrancy of make create-datastore-gcs after partial success — related to System prompt tweak: It shouldn't think that it's a lawyer THIS much lol. #2 above

   The Makefile target offers no --force / cleanup escape hatch. Once you've created a half-baked datastore, you must drop to gcloud or the console to clean it up. Out of scope for this PR, but worth filing as a follow-up if this happens more than once.

Documentation

8. CLAUDE.md IAM list is great — .claude/CLAUDE.md:127

   The per-step required permissions block is genuinely useful (storage.buckets.create, discoveryengine.datastores.create, discoveryengine.documents.import, discoveryengine.engines.create). One micro-suggestion: also call out that the service account needs read access to the bucket for the Discovery Engine import — Vertex AI Search uses its own service agent, and the bucket needs that agent granted roles/storage.objectViewer (or be in the same project, in which case it's automatic). Worth a sentence so a cross-project setup doesn't silently fail at import time.

9. Architecture.md flow — clean update; the split into Bucket Creation/Upload → Datastore Creation → App Creation maps 1:1 with the four Make targets. Nothing to change.

What I liked (carry-over + new)

• The check_bucket_location_compat parametrized table in tests is exemplary — it covers the matrix exhaustively without ceremony.
• test_incompatible_bucket_location_raises_before_api_calls asserts the absence of the datastore creation call, which is the right contract to lock in.
• Splitting the four scripts so each is independently runnable with its own --dry-run continues to pay off — the test files are smaller and the failure modes are isolated.
• _uri_to_filename's rejection of subpaths is good defense-in-depth against someone re-running generate-metadata with a non-flat layout in mind.
• enforce_ascii.py as a standalone module with --check for CI is a clean extraction.

Summary

The previously-flagged blocking items are resolved. The remaining items are polish — none would gate a merge. #1 (name validation) and #2 (partial-failure cleanup) are the two I'd push on if you want one more iteration; the rest are nits.

I did not run make lint / make typecheck / make test myself (sandbox restrictions in this CI environment), so please confirm those pass locally before merging.
• Branch: issue-347/automate-gcs-bucket-creation-uploads

[leekahung]

@claude pr-review

[claude]

Claude finished @leekahung's task in 3m 9s —— View job

---

PR Review

• Read CLAUDE.md and architecture docs
• Review changed files (Makefile, scripts, tests)
• Analyze code quality, security, performance
• Check test coverage and quality
• Review documentation updates
• Post final review

---

Re-reviewing after the latest pushes (2f74170 "Address review feedback; move collection_path to shared utils; switch import reconciliation to FULL; adjust unit tests" and e64e9a8 "Resolve merge conflict"). All four items flagged in the prior round are addressed — calling those out first, then a small new batch.

Resolved since prior review

• Client-side name validation (#1) — scripts/shared.py:9 adds validate_resource_name enforcing ^[a-z0-9][a-z0-9-]{0,62}$, applied as an argparse type on --datastore-name / --datastore-id / --app-name. Covered by test_shared.py:11-21 with happy / invalid parametrization including leading-hyphen and length boundary cases.
• Partial-failure cleanup (#2) — create_datastore_gcs.py:249-272 wraps import_documents in try/except, calls delete_datastore(...) on the new datastore, and surfaces a clear "manual delete required" message if rollback itself fails. test_create_datastore_gcs.py:298-367 covers both branches (test_import_failure_rolls_back_datastore, test_import_failure_with_cleanup_error_reports_manual_step).
• Defensive bucket.location — now (bucket.location or "").upper() in check_bucket_location_compat. (create_datastore_gcs.py:66)
• SEARCH_TIER_STANDARD comment — create_app_gcs.py:53-55 now explains the choice ("Standard is sufficient for keyword + semantic search; upgrade to Enterprise if advanced LLM features are needed.")
• collection_path shared helper — extracted to scripts/shared.py:18 and reused in both scripts and tests. Nice DRY win.
• CLAUDE.md cross-project IAM note — corpus-dataset.md:60 adds the cross-project service-agent / roles/storage.objectViewer note. 👍

New / remaining findings

Higher impact

1. DATASTORE_NAME vs DATASTORE_ID flag naming is inconsistent across targets — backend/Makefile:74, 88

   create-datastore-gcs takes DATASTORE_NAME=<value> and prints Your datastore ID is: <value> — and the next-step hint tells the user to feed that same string back as DATASTORE_ID=<value> into create-app-gcs. Two names for the same string is a small but real footgun when a user is following the four-step recipe. Two options: (a) rename both to DATASTORE_ID (matches the GCP console terminology), or (b) add a one-line Note: this is the same value you passed as DATASTORE_NAME in the create-app-gcs help text / Makefile comment. Fix this →

2. Rollback delete_datastore can hang indefinitely — backend/scripts/create_datastore_gcs.py:168-169

   operation = client.delete_data_store(request=request)
   operation.result()  # no timeout

   If the original import_documents failed because of a network/auth issue, the same conditions can affect the rollback LRO — and operation.result() will block until the process is killed. A modest timeout=120 (or similar) would let the script surface "Rollback timed out, manual cleanup required" rather than hang. The cleanup-error code path you've written (test_import_failure_with_cleanup_error_reports_manual_step) is exactly the right shape for this case once a timeout fires. Fix this →

3. The Search app is created but the production agent never references it — backend/scripts/create_app_gcs.py:147

   The final next-step line says Set VERTEX_AI_DATASTORE_LAWS={args.datastore_id} in your .env — note that's the datastore ID, not the app/engine ID. The agent code uses VertexAISearchRetriever against the datastore directly, so the Search app created in step 4 is, as far as I can tell, only useful for browsing/preview in the GCP console. That's fine, but the four-step recipe doesn't explain why step 4 is needed — a future contributor will reasonably ask "why am I creating an app if the backend doesn't read it?" Worth one sentence in either Architecture.md (step 5) or corpus-dataset.md (RAG pipeline section) explaining that the Search app is for console browsing / future SearchService API use, not for the LangChain retriever path. Documentation only.

Lower impact / nits

4. validate_resource_name rejects leading hyphens but the help text doesn't mention it — backend/scripts/shared.py:9-15, backend/scripts/create_datastore_gcs.py:185, backend/scripts/create_app_gcs.py:88

   The regex ^[a-z0-9][a-z0-9-]{0,62}$ (correctly) rejects leading-hyphen names — but both flag helps say only "Lowercase letters, digits, hyphens (1-63 chars)". A user who tries --app-name -foo will get an ArgumentTypeError with the same generic message. The error message in shared.py:11-14 already mentions "Use lowercase letters, digits, and hyphens only (1-63 chars)" — adding "must begin with a letter or digit" would make the failure self-explanatory. Tiny.

5. FULL reconciliation correctness depends on fresh-datastore invariant — backend/scripts/create_datastore_gcs.py:140

   The comment is accurate: "each ingestion creates a fresh datastore". But if someone ever pulls import_documents out for re-use against an existing datastore (e.g. for a partial reingest after a doc fix), FULL would silently delete every document not in metadata.jsonl. The fresh-datastore guarantee is enforced by AlreadyExists raising on create_data_store, so this is fine today — but worth a sentence in the import_documents docstring noting that callers must guarantee freshness, since the function itself doesn't.

6. enforce_ascii.py reads non-ASCII files twice in the error path — backend/scripts/enforce_ascii.py:104-115 (carry-over)

   Still nbd for ~hundreds of small files. Leaving in the list for completeness.

7. metadata_blob content type application/jsonl — backend/scripts/upload_to_gcs.py:145 (carry-over)

   Canonical media type for JSON Lines is application/x-ndjson. Functionally a no-op since Vertex AI Search reads by URI; pure pedantry.

What I liked (this round)

• The four try/except arms in main() for the import-failure rollback (backend/scripts/create_datastore_gcs.py:249-272) get the contract exactly right: best-effort cleanup, never swallow the original error, fall back to a clear "manual cleanup required" message if cleanup itself fails. The corresponding test coverage (test_import_failure_rolls_back_datastore + test_import_failure_with_cleanup_error_reports_manual_step) locks both branches in.
• scripts/shared.py is a tight, well-scoped module — exactly the right granularity for two helpers shared by two scripts. The dedicated test_shared.py with parametrized happy/sad cases is the cherry on top.
• check_bucket_location_compat's parametrized table covers the matrix exhaustively (US, US-CENTRAL1, EU, EUROPE-WEST1, EU-WEST1 × global, us, eu) and the "raises before API calls" assertion (test_create_datastore_gcs.py:278) is the right invariant to lock in.
• The reconciliation-mode comment on FULL (backend/scripts/create_datastore_gcs.py:137-140) calls out exactly the right invariant ("each ingestion creates a fresh datastore") with a defensible reason for the choice.
• corpus-dataset.md now has both the full four-step recipe with example values and the per-step IAM permission list. This is the kind of doc that saves a contributor an hour on their first run.

Summary

The previously-flagged blocking items are fully resolved. This round's items are all polish / DX: the naming consistency (#1) and the rollback timeout (#2) would each be worth one more iteration, but neither blocks merge. Documentation note (#3) is optional.

I did not run make lint / make typecheck / make test myself (sandbox restrictions in this CI environment), so please confirm those pass locally before merging.
• Branch: issue-347/automate-gcs-bucket-creation-uploads

[leekahung]

@claude pr-review

[claude]

Claude finished @leekahung's task in 3m 23s —— View job

---

PR Review

• Read CLAUDE.md and architecture docs
• Review changed files (Makefile, scripts, tests)
• Analyze code quality, security, performance
• Check test coverage and quality
• Review documentation updates
• Post final review

---

Re-reviewing after the latest three pushes (96d4894 Simplifying comments, 07e7d82 Address PR review feedback and renamed DATASTORE_NAME to DATASTORE_ID, add rollback timeout, update docs, 60cf670 Additional fixes). All four items flagged last round are addressed — calling those out first, then a small set of new things.

Resolved since prior review

• DATASTORE_NAME vs DATASTORE_ID (Format the output #1) — Normalized to DATASTORE_ID everywhere (backend/Makefile:74-86), --datastore-name → --datastore-id in create_datastore_gcs.py:186, and APP_NAME → APP_ID / --app-id for parallel consistency in create_app_gcs.py:78. Help text + console hint now read coherently end-to-end ("Your datastore ID is: ... Next: ... DATASTORE_ID=..." → "create-app-gcs ... APP_ID=...").
• Rollback timeout (System prompt tweak: It shouldn't think that it's a lawyer THIS much lol. #2) — ROLLBACK_TIMEOUT_SECONDS = 120 defined at the module level (create_datastore_gcs.py:29) and threaded through delete_datastore as operation.result(timeout=timeout) (create_datastore_gcs.py:173). The existing cleanup-error test (test_import_failure_with_cleanup_error_reports_manual_step) covers the surrounding code path.
• Search app vs datastore relationship (Frontend design tweaks #3) — Architecture.md:185 now explicitly states "The app is for browsing/previewing the datastore in the GCP console; the backend's LangChain VertexAISearchRetriever queries the datastore directly and does not depend on this app." Same content also added to corpus-dataset.md:55-58. This is the right place for it.
• Leading-hyphen help text (File reorg and show feedback #4) — shared.py:11-15 error message now says "must begin with a letter or digit", and both flag helps (create_datastore_gcs.py:189, create_app_gcs.py:81) match. Self-explanatory failure now.
• enforce_ascii double-read (carry-over Idea: A Vector database #5) — enforce_ascii.py:97-102 now reads once with path.read_text(encoding="utf-8") and catches UnicodeDecodeError from that single read. Resolved.
• FULL reconciliation docstring (carry-over) — import_documents now has a docstring (create_datastore_gcs.py:120-127) explicitly stating "Callers must guarantee the datastore is freshly created (no pre-existing documents)" — exactly the invariant note the prior review asked for.

New / remaining findings

Lower impact / nits

1. _NAME_PATTERN permits trailing hyphens — backend/scripts/shared.py:6

   ^[a-z0-9][a-z0-9-]{0,62}$ accepts names like my-ds- (trailing hyphen). Discovery Engine documents the format as starting with a letter/digit but is silent on trailing hyphens in some places — empirically, the server rejects trailing-hyphen IDs with InvalidArgument. A tighter regex would catch it locally:

   _NAME_PATTERN = re.compile(r"^[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?$")

   Nice-to-have. Tests in test_shared.py:17 already cover the leading-hyphen case; a "trailing-" parameter would round out the constraint coverage. Fix this →

2. test_calls_delete_and_waits doesn't assert the timeout kwarg — backend/tests/test_create_datastore_gcs.py:181

   operation.result.assert_called_once()

   Since the whole point of the new code is to pass timeout=ROLLBACK_TIMEOUT_SECONDS, locking that in with assert_called_once_with(timeout=ROLLBACK_TIMEOUT_SECONDS) prevents a future refactor from silently dropping it. One-line tightening. Fix this →

3. metadata_blob content type application/jsonl — backend/scripts/upload_to_gcs.py:134 (carry-over)

   Canonical media type for JSON Lines is application/x-ndjson. Functionally a no-op since Vertex AI Search reads by URI; pure pedantry, leaving for completeness.

4. No client-side --bucket validation — backend/scripts/upload_to_gcs.py:143-147

   GCS bucket names have their own rules (3-63 chars, lowercase, dots/hyphens/digits/letters, no consecutive dots, no leading dash, etc.). The existing validate_resource_name is almost a subset (it lacks dots and the length floor), so reusing it isn't quite right. Letting GCS surface the error is a defensible choice — the messages are clear — but worth a one-line note in --bucket help text linking to GCS naming rules for self-service debugging.

5. bucket.location casing is unspecified at runtime — backend/scripts/create_datastore_gcs.py:59

   The defensive (bucket.location or "").upper() is correct, and the parametrized test covers both upper-case forms (US, US-CENTRAL1, EU-WEST1, etc.). If GCS ever returns mixed-case strings, the upper-case before-comparison handles it. Good defensive code; flagging only because no test exercises lower-case bucket.location returns — could add ("us-east1", "us") to the compatible parametrize as a regression guard. Truly optional.

Documentation

6. .claude/skills/corpus-dataset.md has uncommitted local changes — appears as M in git status and the local working tree reverts the IAM/permissions block to the pre-Format the output #1-fix state. Just noting — if this is leftover scratch from a CI checkout it's a non-issue; if it's intentional, those uncommitted changes would undo the cross-project note that the previous review explicitly praised. Worth a quick git status confirmation before merge.

What I liked (this round)

• The DATASTORE_NAME → DATASTORE_ID + APP_NAME → APP_ID rename is a small change touching many places, and it's done cleanly: error messages, Makefile help, --flag names, console output, and Architecture.md all read consistently as "ID" throughout. The footgun the prior review flagged is gone.
• ROLLBACK_TIMEOUT_SECONDS = 120 as a module-level constant with the rationale in a comment (create_datastore_gcs.py:26-29) is exactly the right level of explanation — a future reader knows why the timeout exists, not just that it does.
• The new import_documents docstring (create_datastore_gcs.py:121-127) documents the fresh-datastore invariant the FULL reconciliation mode depends on. This is the kind of caller-contract note that prevents subtle bugs years down the line.
• The Architecture.md step 5 "for browsing/previewing... does not depend on this app" sentence is the right granularity — short, future-proof, and resolves the "why am I creating this?" question without overcommitting.
• enforce_ascii.py is now a clean single-read path. Smaller blast radius for the I/O bug.

Summary

Two prior-round items (#1 naming, #2 rollback timeout) are fully resolved; the remaining items are pure polish. Nothing here blocks merge — the strongest candidates for one more pass are #1 (tighten the regex to reject trailing hyphens) and #2 (assert timeout= in the delete test). The uncommitted corpus-dataset.md change (#6) is worth a quick look before merging.

I did not run make lint / make typecheck / make test myself (sandbox restrictions in this CI environment), so please confirm those pass locally before merging.

• Branch: issue-347/automate-gcs-bucket-creation-uploads
•