Repository navigation
feat: send the flow bundle as one xz document per side - #171
Conversation
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
CodeBoarding reviewStatus: 2 changed components See the full change in CodeBoarding. Compared against the merge base: this branch is 1 commit behind graph LR
n_Action_Analysis_and_Authentication_Tools["Action Analysis and Authentication Tools"]
n_Remote_Review_and_Briefing_Sync["Remote Review and Briefing Sync"]
n_Remote_Review_and_Briefing_Sync -- "invokes GitHub client utilities and payload han…" --> n_Action_Analysis_and_Authentication_Tools
classDef added fill:#1f883d,stroke:#0b5d23,color:#ffffff;
classDef modified fill:#bf8700,stroke:#7d4e00,color:#ffffff;
classDef deleted fill:#cf222e,stroke:#82071e,color:#ffffff,stroke-dasharray:5 3;
class n_Action_Analysis_and_Authentication_Tools,n_Remote_Review_and_Briefing_Sync modified;
linkStyle 0 stroke:#7d4e00,stroke-width:2px;
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83e64a45e6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| findings, dropped = dict(body.get("findings") or {}), [] | ||
| while True: | ||
| data = _gzip({**body, "findings": findings}) | ||
| data = _compress({**body, "findings": findings}) |
There was a problem hiding this comment.
When a briefing request exceeds 4.3 MB by many similarly sized findings, this loop now runs the expensive xz preset=9 | PRESET_EXTREME compression once for every PR it removes. Since a request can contain up to 200 findings and the script has a 600-second hard watchdog, a sufficiently oversized request can time out before it ever posts or commits the ledger, causing the same findings to be retried on every sync. Remove findings in batches or use a bounded/binary search so fitting the request requires only a few compression passes.
Useful? React with 👍 / 👎.
ivanmilevtues
left a comment
ivanmilevtues
left a comment
There was a problem hiding this comment.
It is really difficult to udnerstand what is happening from the code itself, so that is why i wrote the comments. Otherwise from what i got it is mainly changing the compression algo + being slightly smarter about how we go about sending things.
| #!/usr/bin/env python3 | ||
| """Call the review-guide and architecture endpoints independently after the review analysis. | ||
|
|
||
| Both requests carry the same analyses, call graphs, PR metadata and changed files. Only architecture |
There was a problem hiding this comment.
hate the naming of this file. I would call it remote_faetures or anything better, they are not closed.
There was a problem hiding this comment.
Agreed, renamed to remote_features.py in 005b3a4. The step is now "Run remote features" (id: remote_features), and problem_history.py and the tests import it under the new name. I kept the closed_features_source input and CLOSED_FEATURES_SOURCE env as they are, since workflows already set them. The thread is on the old path, so the file now lives at scripts/remote_features.py.
| "file": n.file_path, | ||
| "start": n.line_start, | ||
| "end": n.line_end, | ||
| "callable": bool(n.is_callable()), |
There was a problem hiding this comment.
This file in general looks like it is minified. Maybe we should reprompt to not use a single letter paratemeters and so on.
There was a problem hiding this comment.
Fair, it was hard to follow. 005b3a4 rewrites it rather than patching it:
- descriptive names, no single-letter parameters;
- short functions:
_Table/_Rowfor the file table,_calls,_components,_flatten; - a module docstring that says what a request sends, why it's packed this way, and the size limits.
Behaviour is unchanged: the bundle was byte-identical before and after on 42 cases, crate included.
Since your approval:
- 7df8dca reads the analyses through
analysis_reader(fix: build the review diff from Core's parsed analysis, not the document #174), so there's no ad hoc parsing ofanalysis.jsonleft in it; - 9b502c9 adds
_encode, one step that checks both of the service's size limits before anything is sent; - 3c4978e compresses each request once: one shared body for both features, plus architecture's with source only when there is source.
5614db9 to
f83141b
Compare
Svilen-Stefanov
left a comment
Svilen-Stefanov
left a comment
There was a problem hiding this comment.
Note to self before merging:
- Retarget to main once #174 is in (it's still stacked on
feat/analysis-json-v1-reader). - The size check inline, that one is the actual fix.
- PR body is a bit off: an oversized file is dropped per side, not "at neither commit", and the "refused key" test it lists doesn't exist anymore.
- Reply to Ivan on his two threads, his approval is from before the rename.
- Mention the behaviour change: a call to a Python/TS class now lists only
Class.__init__, not bothClass.__init__andClass.
| findings, dropped = dict(body.get("findings") or {}), [] | ||
| while True: | ||
| data = _gzip({**body, "findings": findings}) | ||
| data = _compress({**body, "findings": findings}) |
There was a problem hiding this comment.
blocker-ish: pack() only checks the compressed size against MAX_BUNDLE_BYTES (4.3 MB), but the service refuses a briefing over 32 MiB once decompressed (MAX_BRIEFING_BYTES in closed_functions/handler.py). xz gets roughly 10x here, so a briefing between ~32 and ~43 MiB gets sent whole, rejected, and retried on every sync.
Shouldn't we trim findings against 32 MiB of JSON first and then compress once? That would also take care of Codex's recompress-per-finding point.
f9e8534 to
3c4978e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c4978ea85
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if not kept or e.fits is None or compressions == MAX_COMPRESSIONS: | ||
| raise |
There was a problem hiding this comment.
When findings have different compression ratios, removing the largest entries can discard highly compressible data first and make each ratio estimate too optimistic; if all three attempts remain over the compressed limit, this branch raises even though kept still contains findings and the request without them may fit. That skips the entire briefing update rather than merely omitting additional findings, so the same oversized input is retried on subsequent syncs. Fall back to dropping more findings—at least trying the findings-free request—or use a bounded search that guarantees a fitting subset when one exists.
Useful? React with 👍 / 👎.
3c4978e to
ea7f084
Compare
Large repos got no flows or merge risk: crate's bundle was 24.6 MB of gzip, over the service's 4.3 MB limit, because every fact went four times. 149,315 of its 149,316 analysis edges are call-graph edges, nested relations repeat the top level, and the methods index and the graph's nodes are one table. - `side_document()` writes each commit as one document, `head.json` and `base.json`, in place of the two analyses and two call graphs. It holds a row per method or graph node, calls by caller with their lines, the component tree, and only the analysis edges the calls do not imply. The service's `bundle.expand_side` reads it back into every field a feature reads. - `_compress` is xz -9e instead of gzip: its window spans the bundle, so the base side costs little next to a head that repeats most of it. The briefing request shares it. - A changed file over 300 KB is sent at neither commit (the same limit as source). On crate the bundle is 1.42 MB. Needs CodeBoarding/licensing-aws#56 deployed first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The service reads every call already, so the analysis edges that repeat one were rebuilt on its side from leaf membership and then corrected with hidden_edges. Without them crate's guide holds the same content and its peak memory drops from 1.8 to 1.3 GB, so extra_edges is now just the analysis edges that are no call, and the derivation and hidden_edges go. call_graph() no longer builds what side_document dropped: `callable`, and each site's file and column. It returns each call's lines. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The script calls the service's review-guide and architecture endpoints; nothing about it is closed. scripts/remote_features.py replaces scripts/closed_features.py, and the action step, problem_history's import and the tests follow. The public input closed_features_source keeps its name, since renaming it would break workflows that set it. The rewrite uses descriptive names and short functions: side_document is now a _Table of _Row objects plus _calls, _components and _analysis_edges, and collect() reads changed files through _changed_contents. The module docstring says what a request sends, why it is packed as side documents under xz, and each size limit. The side documents, collect() output and request body are byte-identical to before on 42 cases, crate's head and base with its real call graph among them, and call_graph() reads two real pickles the same. One fix: a parent's members were left out when they equalled its leaves' members, but the service rebuilds a parent from its direct children. A parent holding fewer methods than a child read back with the child's extra methods. It now compares with its children's members. No analysis in the 42 cases hit it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
remote_features.py parsed the legacy analysis.json itself: methods_index, each component's file_methods and every relation's all_edges. It now reads an analysis only through the pinned release's parse_unified_analysis, the same engine that unpickles the call graph beside it. The review analyses both commits with that release, so both are always in the format it reads, and a format change in Core needs no change here. The check that a methods_index key is its file and qualified name goes: the keys are built from Core's method entries. InstalledCoreTests runs in the core-compatibility job against the pinned release. tests/fixtures/analysis holds one analysis as Core 0.15.5 and Core main (9cf50fb9, after #571) wrote it. Each release reads only its own, and every fixture it reads must give the same side document. Checked: on CodeBoarding's own analysis (two baseline syncs, 153 files, 22 components) and this repo's, the side document is byte-identical to the one built from the raw document, and Core main reading the v1 rewrite gives the same bytes as 0.15.5 reading the legacy original. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…sending The service refuses a request on two sizes: what is sent (4.3 MB of xz, the Function URL's limit) and what it decompresses to (256 MiB of JSON for a feature, 32 MiB for a briefing). The Action checked only the first, so a briefing of 32-43 MiB of JSON, which xz takes under 4.3 MB, went out whole, was refused, and went out again on every sync. _encode now enforces both in one step: it measures the JSON, which is cheap, then compresses once. The limits sit together and name the service's constants they follow. The briefing leaves findings out against the JSON limit first, largest first, counted per entry, and only recompresses when the compressed size is still over, aimed by the ratio it just measured, at most 3 compressions in all. It used to recompress once per dropped finding. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The guide and architecture each compressed their own body in parallel, nearly the same bytes twice, and architecture compressed again after dropping source.json. At preset 9 | EXTREME each compressor takes about 700 MB. Both features can send the body without source, so it is compressed once and carries why source is missing, which only architecture reads. Architecture's body with source is compressed only when there is source; when it does not fit, the shared bytes stand in. A run compresses once without source and at most twice with it, and never two at a time. The JSON check needs no ratio guess: the 256 MiB limit is 62 times the 4.3 MB one, so it already turns away whatever could only fit at better than 62:1, and no tighter cut-off holds for every request. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
load_analysis already parsed both sides with the pinned release's
parse_unified_analysis and rolled relations up with its
project_relations_to_level, but the diff then read members, file hashes and
method hashes from the raw document: file_methods as {file_path, methods}
groups and methods_index. Core #571's "v1" document has neither, so once the
Action pins a release with it, the review diff fails with "'str' object has
no attribute 'get'".
The diff now reads only what Core parsed: each component's file_methods,
the root's file index and Core's projected relations at every level. The
review analyses both commits with the pinned release, so both sides are
always in the format it reads, and a format change in Core needs no change
here.
The core-compatibility job rendered the committed .codeboarding/analysis.json,
which after a pin moves can still be in the format of the release before,
which the new one refuses. It now runs InstalledCoreTests: tests/fixtures/
analysis holds one analysis as Core 0.15.5 and Core main (9cf50fb9) wrote it,
each release reads only its own, and every fixture the pinned one reads must
give the same view.
Checked on CodeBoarding's own analysis (two baseline syncs, 22 components,
depth 1 and 2) and this repo's: output and summary are byte-identical to
main's script on the legacy documents with 0.15.5, and to that again on
their v1 rewrites with Core main, where main's script fails.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
b026ec5 to
140f605
Compare
ea7f084 to
175d7cd
Compare
It was squashed, so main holds its change but not its commits.
Large repos get no flows or merge risk. On CodeBoarding/crate#1 (run 42718797329) they were skipped because "its bundle is 24.6 MB, over the service's limit" of 4.3 MB. The bundle sent every fact about four times:
Stacked on #174 (
feat/analysis-json-v1-reader): it reads analyses through #174'sanalysis_reader. Retarget tomainonce #174 merges.Needs CodeBoarding/licensing-aws#56 deployed first: only that service reads xz and side documents. App link: https://app.codeboarding.org/CodeBoarding/licensing-aws/pull/56
What changes (
scripts/remote_features.py, formerlyscripts/closed_features.py)Renamed to
remote_features.py, and rewritten to be readable. The module docstring now says what a request sends, why it is packed this way, and its size limits. The step is "Run remote features". Theclosed_features_sourceinput andCLOSED_FEATURES_SOURCEstay as they are, since workflows already set them.call_graph()returns only what the service reads: each node's file, span and callback flag, and each call's lines (nocallable, no site file or column).side_document()writes each commit as one document,head.jsonandbase.json. These replaceanalysis.json,base/analysis.json,head-cg.jsonandbase-cg.json. Each holds:extra_edges): 1 of 149,316 on crate's base. The service reads the rest as calls.Analyses are read through
analysis_reader(fix: build the review diff from Core's parsed analysis, not the document #174), so a side document is the same whether Core wrote the legacy or the "v1"analysis.json.xz
-9einstead of gzip. xz's window spans the bundle, so the base side costs little next to a head that repeats most of it.problem_history.py's briefing request uses it too.Checked against both of the service's size limits before sending.
_encodemeasures the JSON first, which is cheap, then compresses once:The constants name their source,
MAX_BUNDLE_BYTESandMAX_BRIEFING_BYTESin licensing-awsclosed_functions/handler.py. Before this, only the compressed size was checked. A briefing of 32–43 MiB of JSON, which xz takes under 4.3 MB, was sent whole, refused, and sent again on every sync.Each feature request is compressed once, one at a time. Both features can send the body without source, so it is compressed once. It says why source is missing, which only architecture reads.
The briefing leaves findings out against the JSON limit first, largest envelopes first, counted per entry without serialising again. Then it compresses once. Only when the compressed size is still over 4.3 MB does it compress again, down to 90% of what the measured ratio says fits, at most 3 compressions in all. It used to recompress once per dropped finding.
MAX_FILE_BYTES(300 KB, the limit source already had) now also applies to changed files. A changed file over it is left out at the commit where it is that large; the other commit still sends it. Until nowfiles.jsoncapped the file count, not bytes.Behaviour change: a call to a class is listed once, at its constructor
In the review guide, a call to a Python or TS class is now listed only as
Class.__init__(orconstructor). Before, it was listed twice, asClass.__init__and asClass.callee_keysends a call into a class to its constructor, so it givesClass.__init__.Class.caller → Classis a call, so it no longer arrives as an analysis edge. Only the call graph'sClass.__init__remains.Class;Measured on crate
read_bundle, the guide holds the same content as from today's bundle. The order of some lists follows the call order and differs. Architecture's file graphs are identical.side_documentread back by the service'sexpand_sidereproduces every field a feature reads on 40 cases; of the analysis's edges, those that are no call. That covers crate's two sides with the real graph, and 18 local analyses with no graph and with a synthetic one, including methods sitting in two leaves.53f0bb8e, "v1" at3adb6969, both afteralign_relation_edges. Both give the same side document. The rename and rewrite gave the same bytes as before them.3adb6969's writer and read with fix: build the review diff from Core's parsed analysis, not the document #174's single-form reader. The earlier 12-case check used an older v1 writer and reader.Test plan
python -m unittest discover -s tests: 355 OK, 7 skipped🤖 Generated with Claude Code