LEGLINK-912: Move App Configuration exports to the private link-cac repo - #1813
Open
MikeAtPinnacle wants to merge 3 commits into
Open
LEGLINK-912: Move App Configuration exports to the private link-cac repo#1813MikeAtPinnacle wants to merge 3 commits into
MikeAtPinnacle wants to merge 3 commits into
Conversation
The per-environment exports under Config/ are the deployed environments' real configuration - every key, label and Key Vault reference URI for dev, qa, qa2 and test - and this repository is public. They now live in the private lantanagroup/link-cac repository. The catalog, app-config.yaml, stays here: it describes which keys exist and what they mean, and CODEOWNERS, CodeRabbit and the schema check all key off it sitting at the root. CI here does not read link-cac. There is no token and no checkout, and the dependency runs one way only - link-cac reads this repository, which is free because it is public. Two things rule out the reverse: - Actions logs on a public repository are world-readable. validate_aac_secrets.py quotes the offending value in two of its warnings, and check_required_config.py reports store rows absent from the catalog and the labels they carry. Either would publish what moving these files was meant to stop publishing. - A read token for link-cac in this repository's secrets can be read by anyone with write access here, through a workflow change on a same-repo pull request. That is a larger exposure than any check is worth. So appconfig-secret-scan.yml is deleted rather than repointed - a pull request here cannot change link-cac's exports, so it had nothing to catch on this side - and appconfig-catalog-check.yml keeps only what needs nothing external: the unit tests and validate_app_config_schema.py. Its job is renamed accordingly, from "Validate catalog and required keys" to "Validate catalog schema and rules". A check whose name claims more than it verifies is the failure this tooling exists to catch, and neither workflow is a required status check today, so nothing had to be kept in step with the old name. The cost is real and worth stating: a key marked required: true added to the catalog here is no longer blocked at merge. link-cac runs check_required_config.py on its own pull requests and daily, so drift is caught within a day and posted to Slack, and the fix - adding the rows - belongs in that repository regardless. Run the check locally before merging a catalog change. Tooling: - config_key_matching.default_config_dir resolves the export directory once for every script - --config-dir, then LINK_CAC_CONFIG_DIR, then a sibling ../link-cac/Config clone. Six scripts route through it. - validate_aac_secrets.py exits 2 rather than reporting success when the default location holds no exports. A missing link-cac clone is a gate that did not run, not one that passed. Its default glob narrows to app-config.*.json so a local run scans the same set as CI. - The generated key inventory moves to Scripts/AzureAppConfig/config-key-inventory.json, beside config_symbols.json. Still gitignored. - .githooks/pre-commit keeps only the app-config.yaml schema check; nothing under Config/ can be staged here any more. Removing these files does not remove them from history. Sixteen commits between 2026-07-30 and today carry them, reachable from 29 remote branches. Every historical revision was scanned - 52 in all, with --strict - and none contains a credential: every secret was a Key Vault reference from the first commit onward, so nothing needs rotating. What is in public history is internal topology for dev, qa, qa2 and test - 83 hostnames and four Key Vault names. Whether that warrants a history rewrite is scoped separately and is not decided here; this commit is the forward fix, which is what stops the exposure growing. Testing performed: 42 unit tests pass, including four new cases covering default_config_dir precedence and the missing-export hint. Against a sibling link-cac clone, validate_app_config_schema.py, check_required_config.py (104 required keys across dev/qa/test), validate_aac_secrets.py --strict and reconcile_config_catalog.py all exit 0. Negative runs confirm a bad --config-dir and a missing link-cac each exit 2 with a hint naming LINK_CAC_CONFIG_DIR. The remaining workflow parses and grep confirms no reference to link-cac survives in .github/ outside explanatory comments.
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughEnvironment exports move from this repository to the private ChangesConfiguration validation boundary
Configurable export directory tooling
Secret validation relocation
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
MikeAtPinnacle
marked this pull request as ready for review
August 11, 2026 17:59
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.githooks/pre-commit:
- Around line 22-27: Update the staged-file detection in the pre-commit hook so
deletion of app-config.yaml is included, then check for that deletion and reject
it before the existing empty staged_catalog early return. Preserve the current
handling for added or modified app-config.yaml files.
- Around line 61-63: Update the explanatory comment near the required-key check
to state that link-cac CI reports the check, replacing the inaccurate claim that
both repositories' CI report it. Leave the rationale about not running it in
pre-commit unchanged.
In `@Scripts/AzureAppConfig/validate_aac_secrets.py`:
- Around line 302-312: Add focused tests for main() covering no default export
matches returning findings_mod.EXIT_UNUSABLE, matching default exports
proceeding successfully, and explicitly supplied missing paths retaining their
existing validation behavior. Use temporary files to model the export discovery
cases and mock or disable any network activity.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: aa9dfc2c-cce6-47b7-a14c-224e3502f2a8
📒 Files selected for processing (21)
.githooks/pre-commit.github/CODEOWNERS.github/workflows/appconfig-catalog-check.yml.github/workflows/appconfig-secret-scan.yml.gitignoreConfig/README.mdConfig/app-config.dev.jsonConfig/app-config.qa.jsonConfig/app-config.qa2.jsonConfig/app-config.test.jsonScripts/AzureAppConfig/README.mdScripts/AzureAppConfig/apply_appconfig_tags.pyScripts/AzureAppConfig/check_required_config.pyScripts/AzureAppConfig/config_key_matching.pyScripts/AzureAppConfig/export-appconfigs.batScripts/AzureAppConfig/extract_config_keys.pyScripts/AzureAppConfig/reconcile_config_catalog.pyScripts/AzureAppConfig/tests/test_check_required_config.pyScripts/AzureAppConfig/validate_aac_secrets.pyScripts/README.mdapp-config.yaml
💤 Files with no reviewable changes (6)
- Config/README.md
- .github/workflows/appconfig-secret-scan.yml
- Config/app-config.test.json
- Config/app-config.dev.json
- Config/app-config.qa.json
- Config/app-config.qa2.json
Comment on lines
22
to
27
| staged_catalog=$(git diff --cached --name-only --diff-filter=ACM \ | ||
| | grep -E '^app-config\.yaml$' || true) | ||
|
|
||
| if [ -z "$staged_exports" ] && [ -z "$staged_catalog" ]; then | ||
| if [ -z "$staged_catalog" ]; then | ||
| exit 0 | ||
| fi |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject a staged deletion of app-config.yaml.
--diff-filter=ACM excludes deletions. If the catalog is deleted from the index, staged_catalog is empty and Line 26 exits successfully. Detect the deletion and fail before the early return.
Proposed fix
+if git diff --cached --name-only --diff-filter=D -- app-config.yaml |
+ grep -q '^app-config\.yaml$'; then
+ echo "pre-commit: BLOCKED -- app-config.yaml cannot be deleted."
+ exit 1
+fi
+
staged_catalog=$(git diff --cached --name-only --diff-filter=ACM \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| staged_catalog=$(git diff --cached --name-only --diff-filter=ACM \ | |
| | grep -E '^app-config\.yaml$' || true) | |
| if [ -z "$staged_exports" ] && [ -z "$staged_catalog" ]; then | |
| if [ -z "$staged_catalog" ]; then | |
| exit 0 | |
| fi | |
| if git diff --cached --name-only --diff-filter=D -- app-config.yaml | | |
| grep -q '^app-config\.yaml$'; then | |
| echo "pre-commit: BLOCKED -- app-config.yaml cannot be deleted." | |
| exit 1 | |
| fi | |
| staged_catalog=$(git diff --cached --name-only --diff-filter=ACM \ | |
| | grep -E '^app-config\.yaml$' || true) | |
| if [ -z "$staged_catalog" ]; then | |
| exit 0 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.githooks/pre-commit around lines 22 - 27, Update the staged-file detection
in the pre-commit hook so deletion of app-config.yaml is included, then check
for that deletion and reject it before the existing empty staged_catalog early
return. Preserve the current handling for added or modified app-config.yaml
files.
Comment on lines
+61
to
+63
| # The required-key check is deliberately NOT run here. It compares the catalog against every | ||
| # store's export, which now means reading a second repository - and it would fail on gaps a | ||
| # commit did not introduce and cannot fix. Both repositories' CI report it instead. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the required-key CI ownership statement.
The public workflow now runs unit tests and schema validation only. The PR objective moves required-key checks to link-cac, but this comment says that both repositories report the check. State that link-cac CI reports it instead.
Proposed wording fix
-# commit did not introduce and cannot fix. Both repositories' CI report it instead.
+# commit did not introduce and cannot fix. The `link-cac` CI reports it instead.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # The required-key check is deliberately NOT run here. It compares the catalog against every | |
| # store's export, which now means reading a second repository - and it would fail on gaps a | |
| # commit did not introduce and cannot fix. Both repositories' CI report it instead. | |
| # The required-key check is deliberately NOT run here. It compares the catalog against every | |
| # store's export, which now means reading a second repository - and it would fail on gaps a | |
| # commit did not introduce and cannot fix. The `link-cac` CI reports it instead. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.githooks/pre-commit around lines 61 - 63, Update the explanatory comment
near the required-key check to state that link-cac CI reports the check,
replacing the inaccurate claim that both repositories' CI report it. Leave the
rationale about not running it in pre-commit unchanged.
Comment on lines
+302
to
+312
| requested = args.paths or [default_glob] | ||
| # resolve_paths hands an unmatched pattern straight back, so validate_file reports it as a | ||
| # missing file. That is the right answer for a mistyped path, but not for the default: it | ||
| # now points at a different repository, and the overwhelmingly likely cause is that link-cac | ||
| # is not checked out beside this one - a gate that did not run rather than one that passed. | ||
| paths = resolve_paths(requested) | ||
| if requested == [default_glob] and not any(os.path.exists(p) for p in paths): | ||
| print(f"Error: no App Configuration exports found at {default_glob}.", file=sys.stderr) | ||
| print("They live in the private link-cac repository. Clone it beside link-cloud, set " | ||
| "LINK_CAC_CONFIG_DIR, or pass the paths explicitly.", file=sys.stderr) | ||
| return findings_mod.EXIT_UNUSABLE |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline Scripts/AzureAppConfig/tests --items all --type class,function
rg -n -C 4 'validate_aac_secrets|EXIT_UNUSABLE|default_glob|LINK_CAC_CONFIG_DIR|resolve_paths' \
Scripts/AzureAppConfig/testsRepository: lantanagroup/link-cloud
Length of output: 5035
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- validator structure ---'
ast-grep outline Scripts/AzureAppConfig/validate_aac_secrets.py --items all --type function
printf '%s\n' '--- validator relevant code ---'
sed -n '1,180p' Scripts/AzureAppConfig/validate_aac_secrets.py
sed -n '260,340p' Scripts/AzureAppConfig/validate_aac_secrets.py
printf '%s\n' '--- test file ---'
cat -n Scripts/AzureAppConfig/tests/test_validate_aac_secrets.pyRepository: lantanagroup/link-cloud
Length of output: 13450
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- missing-path behavior and exit constants ---'
sed -n '1,120p' Scripts/AzureAppConfig/config_findings.py
sed -n '250,290p' Scripts/AzureAppConfig/validate_aac_secrets.py
printf '%s\n' '--- test declarations and discovery references ---'
python3 - <<'PY'
import ast
from pathlib import Path
test_path = Path("Scripts/AzureAppConfig/tests/test_validate_aac_secrets.py")
tree = ast.parse(test_path.read_text())
for node in tree.body:
if isinstance(node, ast.ClassDef):
methods = [
child.name for child in node.body
if isinstance(child, (ast.FunctionDef, ast.AsyncFunctionDef))
and child.name.startswith("test_")
]
print(f"{node.name}: {methods}")
PY
printf '%s\n' '--- relevant test references ---'
rg -n 'main\(|resolve_paths|validate_file|EXIT_UNUSABLE|default_glob|app-config\.\*|missing' \
Scripts/AzureAppConfig/tests/test_validate_aac_secrets.pyRepository: lantanagroup/link-cloud
Length of output: 6333
Add tests for default export discovery.
Add focused main() tests for no default export matches (EXIT_UNUSABLE), matching default exports, and explicit missing paths. Use temporary files and no network activity.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Scripts/AzureAppConfig/validate_aac_secrets.py` around lines 302 - 312, Add
focused tests for main() covering no default export matches returning
findings_mod.EXIT_UNUSABLE, matching default exports proceeding successfully,
and explicitly supplied missing paths retaining their existing validation
behavior. Use temporary files to model the export discovery cases and mock or
disable any network activity.
Source: Path instructions
AGENTS.md said nothing about application configuration, which is the one
subsystem where a plausible-looking change is most likely to be wrong: the
catalog and the values are now in different repositories, several of the rules
are counter-intuitive, and CI cannot enforce the most important of them.
The new Application Configuration section records what an agent or a developer
has to know before touching a key:
- Where the catalog, the values and the tooling each live, and how the scripts
find link-cac.
- required and sensitive semantics, including that the schema's `default: true`
is annotation only and never applied, so entries must state it.
- That a required: true key needs rows in every environment export, that CI here
does NOT verify it, and that check_required_config.py must be run locally.
- App Configuration outranking environment variables in both runtimes.
- .NET colon notation versus Java slash notation, and the catalog recording the
dotted form.
- (key, label) identity, why adding a labeled row is safe but moving one is not,
and that a label containing ':' matches nothing.
- The keyvaultref content type, and that any other content type makes the
provider serve the literal {"uri": "..."} string as the value.
Also corrects the Pull Requests section, which listed only TECH_DEBT and
LNK-XXX. pr-title-check.yml accepts six prefixes plus two automation ones, and
LEGLINK is the current project - so an agent following this file titled its PRs
in a form the check rejects. The section now lists all of them, notes that the
colon must be followed by a space, points at the workflow as the authoritative
list, and drops the claim that the ticket must be in the LNK project.
Testing performed: documentation only, no code paths touched. Each prefix listed
was checked against the regex in .github/workflows/pr-title-check.yml, and the
configuration rules against app-config.yaml's embedded schema and the behaviour
of check_required_config.py.
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.
🛠️ Description of Changes
The per-environment App Configuration exports under
Config/were the deployed environments' real configuration — every key, label and Key Vault reference URI for dev, qa, qa2 and test — and this repository is public. They now live in the private lantanagroup/link-cac repository.The catalog,
app-config.yaml, stays here: it describes which keys exist and what they mean, and CODEOWNERS, CodeRabbit and the schema check all key off it sitting at the root.CI here does not read
link-cac. No token, no checkout. The dependency runs one way only —link-cacreads this repository, which is free because it is public. Two things rule out the reverse:validate_aac_secrets.pyquotes the offending value in two of its warnings, andcheck_required_config.pyreports store rows absent from the catalog and the labels they carry. Either would publish what moving these files was meant to stop publishing.link-cacin this repository's secrets can be read by anyone with write access here, through a workflow change on a same-repo PR. That is a larger exposure than any check is worth.So
appconfig-secret-scan.ymlis deleted rather than repointed — a PR here cannot changelink-cac's exports, so it had nothing to catch on this side — andappconfig-catalog-check.ymlkeeps only what needs nothing external: the unit tests andvalidate_app_config_schema.py. Its job is renamed from Validate catalog and required keys to Validate catalog schema and rules, because it no longer checks required keys. Neither workflow is a required status check today, so nothing had to be kept in step with the old name.What reviewers should know changed about enforcement
A key marked
required: trueadded toapp-config.yamlis no longer blocked at merge here.link-cacrunscheck_required_config.pyon its own PRs and daily, so drift is caught within a day and posted to Slack, and the fix — adding the rows — belongs in that repository regardless. Run the check locally before merging a catalog change.Tooling
config_key_matching.default_config_dirresolves the export directory once for every script:--config-dir, thenLINK_CAC_CONFIG_DIR, then a sibling../link-cac/Configclone. Six scripts route through it.validate_aac_secrets.pyexits2rather than reporting success when the default location holds no exports. A missinglink-cacclone is a gate that did not run, not one that passed. Its default glob narrows toapp-config.*.jsonso a local run scans the same set as CI.Scripts/AzureAppConfig/config-key-inventory.json, besideconfig_symbols.json. Still gitignored..githooks/pre-commitkeeps only theapp-config.yamlschema check; nothing underConfig/can be staged here any more.On the files already in public history
Removing them does not remove them from history — sixteen commits between 2026-07-30 and today carry them. Every historical revision was scanned: 52 in all, with
--strict, and none contains a credential. Every secret was a Key Vault reference from the first commit onward, so nothing needs rotating. What is in public history is internal topology for dev, qa, qa2 and test only — 83 hostnames and four Key Vault names. No production configuration is involved.Whether that warrants rewriting this repository's history is scoped separately and is not decided by this PR. This PR is the forward fix, which is what stops the exposure growing.
🧪 Testing Performed
All run from the repository root with
link-caccloned as a sibling:python -m unittest discover Scripts/AzureAppConfig/tests— 42 tests pass, including 4 new cases coveringdefault_config_dirprecedence and the missing-export hint.python Scripts/AzureAppConfig/validate_app_config_schema.py— exit 0.python Scripts/AzureAppConfig/check_required_config.py— exit 0; 104 required keys checked across dev, qa and test.python Scripts/AzureAppConfig/validate_aac_secrets.py --strict— exit 0 over all four exports.python Scripts/AzureAppConfig/reconcile_config_catalog.py— exit 0.LINK_CAC_CONFIG_DIRoverride exercised explicitly, which is the resolution path CI would use.Negative paths, to confirm the checks fail when they should rather than passing quietly:
check_required_config.py --config-dir ./nonexistent→ exit 2, with a hint namingLINK_CAC_CONFIG_DIRand the store to export.validate_aac_secrets.pywith the default pointing at a missing clone → exit 2. Previously this printed "No files to validate" and exited 0, which would have read as a clean scan.Also verified: the remaining workflow parses;
grepconfirms no reference tolink-cacsurvives anywhere in.github/outside explanatory comments; regenerating the key inventory writes to its new location and leaves the tree clean; and the trimmed pre-commit hook validated a stagedapp-config.yamlduring the actual commit for this branch.🧑🔬 Unit Testing
📓 Documentation Updated
CLAUDE.md— the Configuration registry section now states that the exports live inlink-cac, that CI here never reads it, and that the required-key check runs there instead.Scripts/AzureAppConfig/README.md— new section on how the export directory is resolved; the secret-scanning section explains why that scan must not run in this repository.Scripts/README.md,Scripts/AzureAppConfig/export-appconfigs.bat,app-config.yaml— paths and examples updated..github/CODEOWNERS— theConfig/app-config*.jsonentry is dropped;link-cacowns those files under the same pattern in its own CODEOWNERS.Config/README.mdmoves tolink-cacwith the rest of the exports.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores