Conversation
…eated aw-webui is a submodule of aw-server, aw-server-rust and aw-tauri, and the generator walked the tree recursively, so it got a full section under each of them: the v0.14.0b8 notes list the same four aw-webui commits three times. Cleaning that up by hand is what the script's docstring told you to do, and what was done for v0.12.0/v0.13.0. Collect the submodule tree first, keyed by repo name, then render one flat section per repo in `repo_order` (which is what the hand-edited v0.13.0 notes look like). Parents that only bumped a submodule now drop out entirely instead of leaving an empty aw-server header. When parents point at *different* commits of a shared submodule, the section covers the union of the ranges (`git log tip1 tip2 --not base1 base2`) and carries a warning naming what each parent pinned, so a desync shows up in the release notes instead of silently picking one parent's range. Since each parent has its own clone, the log runs in whichever checkout resolves the most pointers, and anything unresolvable is called out with a hint to `git submodule update --init --recursive`. Also drops the dead `remove_duplicates` FIXME (never called, "doesn't work"), adds the newer repos to `repo_order`, and covers the above with tests on a throwaway nested-submodule repo. Output for repos without submodules (gptme) is byte-identical.
Codex Review SummaryThis 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. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 23c9ef05e8
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
🤖 AI code reviewThis PR reworks the changelog generator to collect the submodule tree into a flat, per-repo structure before rendering, so a submodule shared by several parents (aw-webui) is rendered once with the union of its commit ranges and a warning when parents pin different commits. It adds a new test suite and a GitHub Actions workflow to run it, and removes the dead remove_duplicates helper. Needs a look — P2 onlyConfidence 4/5 2 findings ·
|
| commit | score | findings | engine | when |
|---|---|---|---|---|
23c9ef05e88c |
2/5 | 2 | llm | 2026-09-19 11:27 UTC |
472fbf07576e |
3/5 | 3 | llm | 2026-09-19 17:42 UTC |
b9fe8355a7fa |
3/5 | 2 | llm | 2026-09-19 19:02 UTC |
Reviewed 3ff21329ca06 · openrouter/deepseek/deepseek-v4-flash-0731 · llm engine · 81s · about this reviewer
Maintainer commands
@TimeToBuildBob review (own line) — fresh review · @TimeToBuildBob fix — a worker acts on the findings. Once per comment; 👀 = received.
… bumped Review findings on the first commit, all three of them real: - `tip1 tip2 --not base1 base2` is not the union of the ranges. A commit in one parent's range gets dropped if another parent's *base* already contains it, which is exactly what happens when the parents started the release on different commits of the shared submodule. Log each range on its own and merge by commit id instead (ordering by commit date once there is more than one). - `git submodule summary` only reports submodules that moved, so the most likely desync — one parent bumped, another forgotten — produced no warning at all, because the forgotten parent was never collected. Read the current pins from the index separately (`collect_pins`), so every parent that vendors the repo is named in the warning whether or not it moved. - out-of-sync now compares those pins rather than whole ranges, so parents that started apart and converged no longer get a warning claiming they differ while showing identical commits. Also: `_pick_checkout` returns None instead of a path that may not exist (an uninitialized submodule used to crash the run), and the repeated `hidden > 1` test CodeQL flagged is gone. Two new tests cover the dropped-commit and never-bumped cases; both fail on the previous commit. Regenerating v0.14.0b8 gives byte-identical output to before, as does gptme (no submodules).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 472fbf0757
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…lvable ones Second review round, two more real ones: - a parent that adopts a submodule mid-release has no range of its own, so merging its unbounded log with another parent's bump replayed the submodule's entire history into the release notes. Prefer the bounded ranges when there are any; a submodule that is new to *every* parent still lists everything, as before. - when no checkout can resolve a repo's pointers (a shallow clone whose base commit is gone, say), the repo vanished from the notes with only a log line. It now gets its section and the "could not resolve" note, which was the point of that note. Both cases fail on the previous commit.
|
@greptileai review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c58d278639
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… on filtered repos Third review round: - bounding the newly-vendored parent by dropping its range threw away commits that only its pin has (it may adopt the repo at a commit ahead of what the other parents bumped to). Bound it by where the parents that already had it started the release instead, so every pinned commit is covered — which is what the out-of-sync note promises. - a repo whose only commits this release are filtered (build/ci) has no entries, so the section was dropped along with its out-of-sync warning — exactly when the mismatch is least visible otherwise. A repo that moved now keeps its section for the warning alone; one that didn't move stays silent. Both fail on c58d278.
|
@greptileai review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9fe8355a7
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ninitialized dirs Fourth review round: - an uninitialized submodule leaves an empty directory, and git commands run there answer for the *superproject*: `git ls-files --stage` reports the gitlink itself as "./", so `collect_pins` recursed on ever-longer paths (the string-keyed `seen` never matched) until it hit RecursionError. Check that a directory is its own checkout before reading pins from it. - adoption with no accompanying bump left `bounded` empty, so the unbounded range survived and replayed the repo's whole history. Bound the pins by where the parents that already had it sit, whether or not anything was bumped; a repo new to every parent still lists everything. - ranges are now resolved one at a time, each in a checkout that holds it (preferring the clone of the parent it came from), rather than all from one "best" checkout. With shallow clones holding disjoint histories, commits that are available somewhere are no longer reported as unavailable everywhere. Three of the four are Codex's, the adoption one is Greptile's. All fail on b9fe835.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ff21329ca
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for pointer in pointers: | ||
| if pointer.commit_range in resolved: | ||
| continue | ||
| path = _find_checkout([pointer.path] + checkouts, pointer.commit_range) |
There was a problem hiding this comment.
When a newly added submodule is uninitialized, its normalized range is ("", ""), so _resolvable_in succeeds vacuously and this prepended raw pointer.path is selected even though it was excluded from checkouts by _is_checkout. Running git log inside that empty directory then resolves to the enclosing superproject and attributes its entire history to the new submodule (or fails if the directory is absent). Fresh evidence beyond the earlier pin-collection fix is that _resolve_ranges reintroduces the unchecked path; restrict every candidate here to verified checkouts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed together with the other fresh Codex findings in TimeToBuildBob/activitywatch@39783ce. _resolve_ranges now passes only _is_checkout-verified candidates, so an empty uninitialized gitlink directory cannot win vacuously and fall back to the superproject. Added a focused regression test.
| starts = [since for _, (since, _) in bounded] or [ | ||
| pin.commit for pin in repo.pins | ||
| ] | ||
| if len(starts) > 1 or bounded: |
There was a problem hiding this comment.
When two parents both add the same repository during this release, bounded is empty but starts contains one pin per parent, so this condition treats the repository as if it had pre-existing parents. If both parents pin the same commit, the generated ranges are pin...pin and the entire repository section disappears; with different pins, all history through the selected base is omitted. Fresh evidence beyond the prior single-parent-adoption fix is the all-parents-new case; when there is no bounded range, retain one unbounded range so the newly introduced repository's history is listed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in TimeToBuildBob/activitywatch@39783ce. The all-parents-new case now stays unbounded when every current pin agrees, while adoption into an already-vendored but unmoved repo still uses the older existing pin as its boundary. Added test_repo_new_to_every_parent_keeps_full_history; focused suite is 17/17.
| path = _find_checkout([pin.path] + checkouts, (base, pin.commit)) | ||
| if path: | ||
| bounded.append((path, (base, pin.commit))) |
There was a problem hiding this comment.
When an existing parent's bounded range is in one shallow checkout and a newly vendored pin is in another checkout lacking that range's base, _find_checkout returns None here and the new pin is silently skipped. The original empty range was already considered resolved and _sync_notes ran before this derived range was attempted, so the release notes neither include the pin's unique commits nor warn that they are missing. Fresh evidence beyond the earlier per-range resolution fix is that these synthetic bounded ranges are created afterward; failed bounds must be carried into the unresolved diagnostics.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in TimeToBuildBob/activitywatch@39783ce. Failed synthetic (base, new-pin) resolutions are now appended to the unresolved diagnostics instead of silently skipped, and notes are rebuilt after deriving those ranges. Added test_unresolvable_synthetic_adoption_range_gets_warning; full focused suite passes 17/17.
| f"git log {rev} --no-decorate --pretty={pretty}", cwd=path | ||
| ).split("\n"): | ||
| if line: | ||
| _id, timestamp, _author, email, msg = line.split("\t") |
There was a problem hiding this comment.
_log_commits, the pretty format string is format:'%h%x09%ct%x09%an%x09%ae%x09%s' and each line is split with line.split("\t"). The commit subject (%s) is the last field, so a subject containing a tab character would cause the split to produce more than 5 fields, and the unpacking _id, timestamp, _author, email, msg = line.split("\t") would raise a ValueError, aborting the whole changelog generation. Git commit subjects can contain tabs (they are not prohibited), so a single commit with a tab in its subject would crash the script. The old code had the same issue, but this PR rewrites the logging path and the new tests never exercise a subject with a tab. The fix is to split with maxsplit=4 or to use a more robust delimiter.
There was a problem hiding this comment.
Same defect as the earlier thread (fp 03f198b0a170, answered at 19:46Z) — the reviewer regenerated it under a new fingerprint on 3ff2132, so it will keep reappearing while this head stands.
The fix is on TimeToBuildBob/activitywatch@39783ce (branch bob/fix-changelog-review-round4): _log_commits splits with line.split("\t", 4) plus a test_commit_subject_with_tab_does_not_break_log_parsing regression test. It is not on this PR's head because TimeToBuildBob has no write access to the upstream branch (and no ResolveReviewThread permission either), so it lands only when Erik cherry-picks that commit. Leaving this thread open is the accurate signal until then.
| return repos | ||
| seen.add(path) | ||
|
|
||
| for name, commit in _submodule_pins(path): |
There was a problem hiding this comment.
_sync_notes, the out-of-sync warning is based on repo.out_of_sync, which compares the set of pin.commit values. The pins are collected by collect_pins, which reads the current index of each checkout. However, collect_pins only records pins for submodules that are actually checked out (via _is_checkout). If a parent's submodule is deinitialized (as in the test test_uninitialized_submodule_does_not_derail_pin_collection), that parent's pin is not recorded, so the warning may be missing even though the parent's gitlink in the superproject's index pins a different commit. The test only checks that the deinitialized submodule doesn't derail collection, not that the warning is correct. In a real release, if a parent's submodule is not initialized, the changelog will not warn about that parent being out of sync, even though the release notes should. The fix would be to read pins from the superproject's index (git ls-files --stage) rather than from the submodule's own checkout, but the current design relies on the checkout being present.
There was a problem hiding this comment.
False positive — the premise does not hold. collect_pins starts at the superproject root, and _submodule_pins(parent) reads gitlinks from the parent's index (git ls-files --stage in the parent), not from the pinned repo's own checkout. _is_checkout guards the directory being scanned (so an empty leftover directory can't answer for the superproject), not whether the pinned repository is initialized.
Reproduced locally: after git submodule deinit -f server, the root index still reports 160000 <sha> 0 server, so the root's pin is recorded and out_of_sync still sees it. The only thing a deinitialized submodule loses is deeper recursion into its own gitlinks — which is unreadable without its objects regardless, so no warning is silently dropped.
("How this was verified" states the test "only checks that the deinitialized submodule doesn't derail collection, not that the warning is correct" — true of the test, but the code path above is the reason the warning is correct anyway.)
|
The remaining fix is ready but cannot be pushed to this upstream-owned branch: cherry-pick |
3 participants
The v0.14.0b8 release notes list the same four aw-webui commits three times: aw-webui is a submodule of aw-server, aw-server-rust and aw-tauri, and
build_changelog.pyrenders the submodule tree recursively, so each parent gets its own full aw-webui section. The script's own docstring told you to fix this by hand ("Remove duplicate aw-webui entries"), which is what the v0.12.0/v0.13.0 notes were: a flat, deduplicated list.What changed
Instead of rendering while walking, the tree is collected first (
collect_repos), keyed by repo name, then rendered flat: one section per repo, ordered byrepo_order, root first. That is the shape the hand-edited v0.13.0 notes have.Side effects, all wanted:
## 📦 aw-serverheader (as in b8)repo_ordergained the repos added since it was written (aw-tauri, aw-notify, aw-watcher-input, awatcher, media), so they no longer land in arbitrary orderremove_duplicatesFIXME ("Doesn't work, messy af") is gone — it was never calledWhen the parents are out of sync
This is the interesting case, and the reason for not simply keeping the first occurrence. Until now the parents happened to agree (a
build(deps): updated submodulescommit bumps them together), but they disagree whenever one parent is bumped and another isn't — which is exactly the case where dropping a section would hide real changes.So a shared submodule is rendered once with the union of the ranges —
git log tip1 tip2 --not base1 base2, i.e. everything new in any parent — and the section carries a warning naming what each parent pinned:A desync becomes visible in the release notes (and a
logger.warningduring generation) instead of being silently resolved to one parent's range. Nothing is dropped, nothing is double-counted.Each parent has its own clone of the submodule, so a sibling's commits may not exist in any given one. The log therefore runs in whichever checkout resolves the most pointers, and anything still unresolvable gets its own note:
Verification
scripts/tests/test_build_changelog.pybuilds a throwaway bundle/server/server-rust/webui tree and covers dedup, flat ordering, the dropped parent, the out-of-sync union + warning, and the absence of a warning when in sync; run in CI by.github/workflows/changelog-tests.ymlThe remaining manual step for a release is what it always was, and the docstring now says so: writing the
## Summaryhighlights.