feat: inventory and ratchet first-party private API access - #1078
Merged
Merged
Conversation
Hypermedia surface change (
|
There was a problem hiding this comment.
🟡 Changes recommended
The scanner currently constructs invalid AST nodes (missing ast.Name.ctx) and ledger validation can crash on malformed entries, which would break the ratchet’s reliability in CI.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a maintained inventory and CI-enforced ratchet for first-party production usage of Chirp private APIs, backed by a pinned downstream audit ledger and an offline static scanner (no downstream code execution).
Changes:
- Introduce an AST-based scanner + ratchet script (
scripts/private_api_ratchet.py) and a pinned five-repository ledger (scripts/private_api_ledger.json). - Add focused tests proving provenance tracking, boundaries, and failure modes (
tests/test_private_api_ratchet.py). - Wire validation into standard CI and add a path-scoped workflow that checks out and parses the pinned downstream repos (
.github/workflows/private-api-audit.yml).
File summaries
| File | Description |
|---|---|
| tests/test_private_api_ratchet.py | Adds offline fixtures covering scanner provenance, dynamic attribute detection, receiver hint behavior, and ratchet failures. |
| scripts/private_api_ratchet.py | Implements the static scanner, ledger validation, and per-repo ratchet checks with a CLI surface for CI/workflows. |
| scripts/private_api_ledger.json | Adds the pinned 5-app ledger with classifications, ownership, rationale, follow-ups, counts, and source links. |
| docs/design/first-party-private-api-inventory.md | Documents the inventory/ratchet intent, workflow model, and scanner boundaries/review process. |
| changelog.d/1053.added.md | Changelog entry for the new ledger + ratchet + audit workflow. |
| .github/workflows/private-api-audit.yml | Adds a path-scoped workflow that checks out pinned downstream revisions and runs the ratchet. |
| .github/workflows/ci.yml | Adds a CI step to validate the private API ledger via the ratchet script. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+191
to
+192
| for arg in [*node.args.posonlyargs, *node.args.args, *node.args.kwonlyargs]: | ||
| self._bind(ast.Name(id=arg.arg), self.origin(arg.annotation)) |
Comment on lines
+290
to
+300
| prefix = f"https://github.com/{repository['repository']}/blob/{revision}/" | ||
| if not finding.get("source", "").startswith(prefix): | ||
| errors.append(f"{name}: {key}: source must link the pinned revision.") | ||
| if finding.get("classification") == "test-only" and not _test_path(key.split("|")[0]): | ||
| errors.append( | ||
| f"{name}: production access cannot use test-only classification: {key}." | ||
| ) | ||
| if type(finding.get("count")) is not int or finding["count"] < 1: | ||
| errors.append(f"{name}: {key}: count must be positive.") | ||
| prefix = f"https://github.com/{repository['repository']}/blob/{revision}/" | ||
| errors.extend( |
Comment on lines
+319
to
+322
| def check_repository(root: Path, repository: dict[str, Any]) -> list[str]: | ||
| findings = scan_repository(root, repository) | ||
| expected = {row["key"]: row for row in repository["findings"]} | ||
| counts = Counter(row.key for row in findings if not _test_path(row.path)) |
There was a problem hiding this comment.
🟡 Changes recommended
The new ratchet script has a few robustness/runtime hazards (notably AST node construction and validation paths that can raise KeyError) that should be fixed before relying on it in CI.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
scripts/private_api_ratchet.py:192
ast.Namenodes require actxfield in the stdlib AST API; constructingast.Name(id=...)can raise aTypeErrorat runtime on many Python versions (and is unnecessary here). Pass an explicitast.Load()context so argument-binding provenance scanning can’t crash.
for arg in [*node.args.posonlyargs, *node.args.args, *node.args.kwonlyargs]:
self._bind(ast.Name(id=arg.arg), self.origin(arg.annotation))
scripts/private_api_ratchet.py:292
validate_ledger()indexesrepository['repository']while still in the validation phase; a malformed ledger will raiseKeyErrorinstead of returning actionable validation errors. Use.get(...)here so validation stays non-throwing.
prefix = f"https://github.com/{repository['repository']}/blob/{revision}/"
if not finding.get("source", "").startswith(prefix):
errors.append(f"{name}: {key}: source must link the pinned revision.")
scripts/private_api_ratchet.py:299
- Same
KeyErrorrisk as above:validate_ledger()builds a prefix viarepository['repository']while validating. Using.get(...)keeps validation robust for partially-edited ledgers.
prefix = f"https://github.com/{repository['repository']}/blob/{revision}/"
scripts/private_api_ratchet.py:322
check_repository()assumesrepository['findings']exists; if the ledger is missing that key (or is mid-edit), this will crash instead of producing a clear validation error. Preferrepository.get('findings', [])and letvalidate_ledger()be the authoritative gate.
def check_repository(root: Path, repository: dict[str, Any]) -> list[str]:
findings = scan_repository(root, repository)
expected = {row["key"]: row for row in repository["findings"]}
counts = Counter(row.key for row in findings if not _test_path(row.path))
scripts/private_api_ratchet.py:379
--matrixoutput still iteratesledger['repositories']directly; if the ledger is missing that key, this will crash. Use the already-derivedrepositoriesmapping (orledger.get(...)) so--matrixstays non-throwing and can show ledger validation errors cleanly.
if args.matrix:
print(
json.dumps(
{
"include": [
{
"name": name,
"repository": repo["repository"],
"revision": repo["revision"],
}
for name, repo in ledger["repositories"].items()
]
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+346
to
+353
| ledger = json.loads(args.ledger.read_text()) | ||
| errors = validate_ledger(ledger) | ||
| for spec in args.repo: | ||
| name, separator, directory = spec.partition("=") | ||
| if not separator or name not in ledger["repositories"]: | ||
| errors.append(f"Unknown repository mapping {spec!r}; use NAME=PATH from the ledger.") | ||
| continue | ||
| repository = ledger["repositories"][name] |
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
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.
Production applications currently depend on private Chirp APIs without a consistent inventory or gate for new dependencies. This adds a pinned five-application ledger covering 51 production accesses across 45 identities, with owners, rationale, and follow-ups, plus a static ratchet that rejects new unclassified production access.
The scanner follows import provenance, annotations, aliases, and reviewed receiver hints. Offline fixtures and ledger validation run in ordinary CI; a separate path-scoped workflow parses all five pinned downstream repositories without installing or executing them. Dynamic names and control-flow-dependent rebinding remain explicit manual-review boundaries.
Validation: 31 focused tests pass with 89% scanner coverage; all-repository Ruff and formatting pass; all five pinned source audits pass, including a clean Pidge checkout.
Closes #1053
Advances-Epic #1052
Acceptance #1053: tests/test_private_api_ratchet.py uses @pytest.mark.issue(1053); committed source pins and owner/follow-up mappings document the audited production boundary.