FE-1309: Stop committing generated Petrinaut optimizer client types - #9147
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9147 +/- ##
==========================================
- Coverage 59.57% 59.57% -0.01%
==========================================
Files 1413 1413
Lines 138053 138053
Branches 6510 6510
==========================================
- Hits 82242 82240 -2
- Misses 54769 54771 +2
Partials 1042 1042 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
PR SummaryMedium Risk Overview
Root Reviewed by Cursor Bugbot for commit e7ff90c. Bugbot is set up for automated code reviews on this repo. Configure here. |
`src/openapi.gen.ts` is derived entirely from the committed spec at `apps/petrinaut-opt/openapi/openapi.json` by the package's `codegen` script, and was force-added past the root `.gitignore` rule for `*.gen.*`. Untrack it; the ignore rule already covers it, and every task that reads it depends on `codegen`. The `git diff --exit-code` clause in the client's `test:unit` ran after `yarn codegen` in a package where turbo already sequences `codegen` first, so it could never report a difference. Drop it. The equivalent clause in `apps/petrinaut-opt` guards the committed spec and stays. Give `@apps/petrinaut-website` a `lint:tsc` task depending on `^build`. The root definition depends on `codegen` and `build:types`, neither of which the website has anywhere in its chain, so `turbo run lint:tsc --filter "@apps/petrinaut-website"` never scheduled the client's `codegen`. Mark the optimizer OpenAPI spec and the generated architecture diagrams as `linguist-generated` so they collapse in diffs.
7c68653 to
0250f58
Compare
e7ff90c to
0250f58
Compare
Benchmark results
|
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| resolve_policies_for_actor | user: empty, selectivity: high, policies: 2002 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: medium, policies: 1002 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: high, policies: 3314 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: medium, policies: 1527 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: high, policies: 2078 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: medium, policies: 1033 | Flame Graph |
policy_resolution_medium
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| resolve_policies_for_actor | user: empty, selectivity: high, policies: 102 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: medium, policies: 52 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: high, policies: 269 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: medium, policies: 108 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: high, policies: 133 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: medium, policies: 63 | Flame Graph |
policy_resolution_none
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| resolve_policies_for_actor | user: empty, selectivity: high, policies: 2 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: medium, policies: 2 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: high, policies: 8 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: medium, policies: 3 | Flame Graph |
policy_resolution_small
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| resolve_policies_for_actor | user: empty, selectivity: high, policies: 52 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: medium, policies: 26 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: high, policies: 94 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: medium, policies: 27 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: high, policies: 66 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: medium, policies: 29 | Flame Graph |
read_scaling_complete
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| entity_by_id;one_depth | 1 entities | Flame Graph | |
| entity_by_id;one_depth | 10 entities | Flame Graph | |
| entity_by_id;one_depth | 25 entities | Flame Graph | |
| entity_by_id;one_depth | 5 entities | Flame Graph | |
| entity_by_id;one_depth | 50 entities | Flame Graph | |
| entity_by_id;two_depth | 1 entities | Flame Graph | |
| entity_by_id;two_depth | 10 entities | Flame Graph | |
| entity_by_id;two_depth | 25 entities | Flame Graph | |
| entity_by_id;two_depth | 5 entities | Flame Graph | |
| entity_by_id;two_depth | 50 entities | Flame Graph | |
| entity_by_id;zero_depth | 1 entities | Flame Graph | |
| entity_by_id;zero_depth | 10 entities | Flame Graph | |
| entity_by_id;zero_depth | 25 entities | Flame Graph | |
| entity_by_id;zero_depth | 5 entities | Flame Graph | |
| entity_by_id;zero_depth | 50 entities | Flame Graph |
read_scaling_linkless
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| entity_by_id | 1 entities | Flame Graph | |
| entity_by_id | 10 entities | Flame Graph | |
| entity_by_id | 100 entities | Flame Graph | |
| entity_by_id | 1000 entities | Flame Graph | |
| entity_by_id | 10000 entities | Flame Graph |
representative_read_entity
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/block/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/book/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/building/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/organization/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/page/v/2
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/person/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/playlist/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/song/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/uk-address/v/1
|
Flame Graph |
representative_read_entity_type
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| get_entity_type_by_id | Account ID: bf5a9ef5-dc3b-43cf-a291-6210c0321eba
|
Flame Graph |
representative_read_multiple_entities
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| entity_by_property | traversal_paths=0 | 0 | |
| entity_by_property | traversal_paths=255 | 1,resolve_depths=inherit:1;values:255;properties:255;links:127;link_dests:126;type:true | |
| entity_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:0;links:0;link_dests:0;type:false | |
| entity_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:0;links:1;link_dests:0;type:true | |
| entity_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:2;links:1;link_dests:0;type:true | |
| entity_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:2;properties:2;links:1;link_dests:0;type:true | |
| link_by_source_by_property | traversal_paths=0 | 0 | |
| link_by_source_by_property | traversal_paths=255 | 1,resolve_depths=inherit:1;values:255;properties:255;links:127;link_dests:126;type:true | |
| link_by_source_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:0;links:0;link_dests:0;type:false | |
| link_by_source_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:0;links:1;link_dests:0;type:true | |
| link_by_source_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:2;links:1;link_dests:0;type:true | |
| link_by_source_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:2;properties:2;links:1;link_dests:0;type:true |
scenarios
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| full_test | query-limited | Flame Graph | |
| full_test | query-unlimited | Flame Graph | |
| linked_queries | query-limited | Flame Graph | |
| linked_queries | query-unlimited | Flame Graph |
Requested by Chris Feijoo · Slack thread
🌟 What is the purpose of this PR?
libs/@local/petrinaut-optimizer-client/src/openapi.gen.tsis a generated file that was nonetheless tracked in git. It is the only tracked*.gen.*in the repo — the root.gitignorehas covered*.gen.*since long before this file existed, and it was force-added past that rule.Before. You clone the repo and the generated client types are already sitting there in your worktree. Change anything about the optimizer's OpenAPI spec and you get two diffs to review: the one you wrote, and 500-odd lines of mechanical TypeScript underneath it. Rebase two branches that both touched the spec and you conflict in the generated file as well as the real one. Meanwhile the check that was supposed to keep the file honest —
yarn codegen && git diff --exit-code -- src/openapi.gen.ts, wired into the package'stest:unit— could never fail, because turbo already runscodegenbeforetest:unitin that package, so the working tree is regenerated before the diff is taken.After. You clone the repo and the file is not there. The first turbo task that needs it builds it, because
codegenis already a declared dependency ofbuild,lint:tscandtest:unit, with the spec as its input and the file as its output — so turbo caches it and rebuilds it exactly when the spec changes. Spec changes now produce one diff. The optimizer spec itself and the generated architecture diagrams are markedlinguist-generated, so GitHub collapses them by default too.This is the arrangement
@local/hash-graph-clientalready uses: commit the spec, mark it generated, gitignore the TypeScript derived from it.🔗 Related links
🔍 What does this change?
src/openapi.gen.tsviagit rm --cached. The file stays on disk;.gitignore:130(*.gen.*) already covers it, so no new ignore rule was needed.test:unit, leavingvitest --run src. Thegit diff --exit-codeclause is now a no-op on an untracked file, and was already a no-op before that for the reason above. The equivalent clause inapps/petrinaut-opt/package.jsonis deliberately left alone — that one guards the committed spec, which is a real source of truth and does need to stay in sync with the Python app.lint:tsctask toapps/petrinaut-website/turbo.jsondepending on^build. This is the load-bearing part: the rootturbo.jsondefineslint:tsc.dependsOn = ["codegen", "build:types"], but@apps/petrinaut-websitehas neither script and nothing in its chain providesbuild:types, so the literal CI command (turbo run lint:tsc --filter "@apps/petrinaut-website",.github/workflows/lint.yml:130) never scheduled the client'scodegen. On a clean checkout it would have typechecked against a missing file.^buildreaches@local/petrinaut-optimizer-client#build, which depends on that package'scodegen..gitattributes:apps/petrinaut-opt/openapi/openapi.json, and the.d2/.svgfiles underlibs/@hashintel/petrinaut-core/docs/architecture/produced byscripts/generate-dependency-diagrams.mjs.This PR will run CI completely cold, and unrelated flakes may surface. The root
turbo.jsonlists.*inglobalDependencies, so editing.gitattributesinvalidates the entire turbo cache — every task in the repo rebuilds from scratch on this branch. A red check here is worth a second look before it is attributed to the diff; compare against the same job onmain.On a fresh clone the generated file is absent until something builds it.
libs/@local/petrinaut-optimizer-client/src/openapi.gen.tsdoes not exist until the first turbobuild,lintortestrun produces it, so until then an editor's language server reports unresolved imports in that package and inapps/petrinaut-website. This is the cost of not tracking the file; running any of those tasks once resolves it for good.🛡 What tests cover this?
Nothing new. The removed
git diff --exit-codeclause was not providing coverage — it could not fail. Real drift protection lives inapps/petrinaut-opt'stest:unit, which regenerates the spec from the Python app and diffs it, and is untouched here.❓ How to test this?
.gitattributesglobs match the four intended files and nothing else:Verified locally: re-running the exact
codegencommand by hand (openapi-typescript@7.13.0+oxfmt@0.50.0, matching the pinned devDependencies) reproduces the previously committed file byte-for-byte.Pre-Merge Checklist 🚀
🚢 Has this modified a publishable library?
This PR:
📜 Does this require a change to the docs?
The changes in this PR:
🕸️ Does this require a change to the Turbo Graph?
The changes in this PR:
turbo.json's have been updated to reflect this