fix(review): let a base outlive the reviews that reference it - #92
Conversation
The renewal rule added for dangling references republished a base whenever the one it found expires within a review's lifetime. Base retention was also 30 days, the same as a review's, so a fetched base always had less than that left and the rule fired on every run: every review republished the base graph, which is the duplication that publishing it separately removed. The optimisation was inert. Base graphs are now kept 90 days. Correctness still comes from the renewal check, so a review never names a base that expires under it; the margin only decides how often renewal fires, which is now at most once per sixty days per merge base instead of once per run. A test asserts base retention exceeds review retention, since equal lifetimes are what made the rule degenerate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 576695a)
CodeBoarding reviewStatus: 0 changed components See the full change in CodeBoarding. graph LR
n_Visual_Rendering_Engine["Visual Rendering Engine"]
n_Structural_Diff_Engine["Structural Diff Engine"]
n_Interaction_Orchestrator["Interaction Orchestrator"]
n_Visual_Rendering_Engine -- "Returns rendering metadata and diagram artifacts" --> n_Interaction_Orchestrator
n_Structural_Diff_Engine -- "Provides annotated diff model for visualization" --> n_Visual_Rendering_Engine
n_Interaction_Orchestrator -- "Triggers structural comparison via CLI" --> n_Structural_Diff_Engine
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;
|
![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: 37caf810ac
ℹ️ 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".
| # renewal check, which republishes any base with less than a review's | ||
| # lifetime left; this margin is what decides how often that fires. Equal | ||
| # lifetimes would renew on every run and undo the deduplication. | ||
| retention-days: 90 |
There was a problem hiding this comment.
In repositories whose organization or repository policy caps artifacts at 30 days or less—a supported case explicitly noted in README.md:91—GitHub shortens both this 90-day request and the review artifact's 30-day request to the same lifetime. Because the base upload precedes the review upload in this workflow, the referenced base then expires slightly earlier, and every subsequent run also renews it because fetch-state.sh sees less than 30 days remaining. Thus the deduplication and outliving guarantees still fail in these environments; use the effective retention limit to shorten/in-line the review or otherwise retain a real margin rather than comparing only the requested YAML values.
Useful? React with 👍 / 👎.
| # renewal check, which republishes any base with less than a review's | ||
| # lifetime left; this margin is what decides how often that fires. Equal | ||
| # lifetimes would renew on every run and undo the deduplication. | ||
| retention-days: 90 |
There was a problem hiding this comment.
This changes the storage and lifecycle contract without updating the user-facing documentation: README.md:91 still says the action requests 30-day retention, while docs/COMMIT_STRATEGY.md:31 lists bases as retained for 30 days and lines 74-76 describe them as using the repository default. Consumers estimating billed artifact storage or implementing readers from that contract will therefore be given incorrect retention information.
Useful? React with 👍 / 👎.
90 was more headroom than the behaviour needs. The reuse window only has to outlast a pull request, so 60 days leaves 30 after the renewal threshold: a pull request open a month never triggers a republish, and it costs a third less than 90 to store. Modelled at the largest repository we have, 50 pull requests a month with five runs each: 30-day bases cost 173 MB of monthly average because they republish per run, 60-day bases cost 69 MB because they republish per merge base, and 90 cost 104 MB for no additional reuse. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Dropped from 90 to 60 after modelling it — 90 was more headroom than the behaviour needs. The reuse window only has to outlast a pull request. 60 days leaves 30 after the renewal threshold, so a pull request open for a month never triggers a republish, and it stores a third less than 90. Per-push reviews, 50 pull requests a month, largest repository we have:
Worth recording why 30 is the expensive row rather than the cheap one: at 30 the renewal fires on every run, so a base is published per run (250/month) instead of per merge base (50/month). Retention length is not what drives the cost here, publication frequency is. 30 only wins when a pull request gets exactly one review, which is the world before |
1 participant
Follow-up to #90, which merged before this landed on the branch.
The defect
#90 added a renewal rule so a review never names a base that expires under it: if the base I fetched expires within a review's lifetime (30 days), republish it. It also set base retention to 30 days — the same as a review's.
A fetched base therefore always has less than 30 days left, so the rule is always true:
Every review republished the base graph. That is exactly the duplication that publishing the base separately removed — measured at half the review artifact — so the optimisation was inert from the moment the renewal rule was added.
The fix
Base graphs are kept 90 days, reviews stay at 30.
Correctness is unchanged and still comes from the renewal check, so a review never names a base that expires under it. The margin only decides how often renewal fires: a base is now reusable for its first 60 days and renewed only in its last 30, so at most once per sixty days per merge base instead of once per run.
test_a_base_outlives_the_reviews_that_reference_itnow asserts base retention exceeds review retention rather than equalling a fixed number, since equal lifetimes are what made the rule degenerate. Mutation-checked by setting them equal.How it surfaced
Not from the tests — they passed throughout, because the behaviour was correct and merely wasteful. It came out of a question about the webview's fallback policy: when a
base_artifact_id404s the webview deliberately does not retry by name, since a same-name artifact is by construction a different graph. That is the right call, and it puts the burden on the action to keep referenced bases alive — which is what sent me back to check whether the guarantee held, and how.🤖 Generated with Claude Code