Skip to content

feat: save the default branch's analysis to a branch of its own, and generate the review's base in a step of its own - #143

Open
ivanmilevtues wants to merge 18 commits into
mainfrom
refactor/review-baseline-pipeline
Open

ivanmilevtues wants to merge 18 commits into
mainfrom
refactor/review-baseline-pipeline

Conversation

@ivanmilevtues

@ivanmilevtues ivanmilevtues commented Oct 10, 2026 •

Copy link
Copy Markdown
Member

Stacked on #139 → #140 → #142, but targeted at main so the whole stack can be read as one diff.

  • 1fb65b1 is the refactor; review it alone with git show 1fb65b1.
  • 17cd1e8 merges our own workflows.
  • 6597352, c4820ee and 381a0c4 are the input API, from review.
  • 08d55da and f1ebc0c are from the second and third reviews: failure reasons, the deprecated force_full, and sync of the default branch only.

Overview: how a run is orchestrated

There is one action.yml, and every step is gated on mode (review or sync). The only scripts that make decisions are listed below; the rest is machinery.

Shared start

Step File Decides
Translate deprecated inputs old_api_migrator.sh Validates codeboarding_analysis_location and rewrites the deprecated inputs into it, or fails with a link to the migration steps.
Resolve event resolve-github-event.sh Whether to run at all, what to check out, and where the analysis lives: codeboarding/baseline, or in place on the code branch. In branch mode, a sync on any branch but the repository's default fails here, in seconds.
Check LLM configuration verify-credentials.sh Fails within seconds on bad credentials, before the engine is installed.
Check analysis branch (sync) check-analysis-branch.sh Before the checkout and the install: codeboarding/baseline must not exist yet, or be an analysis branch rather than a code branch with that name; a branch named codeboarding, next to which git cannot create it, fails too.
Resolve analysis cache keys resolve-cache-keys.sh The configuration hash (engine version, model, depth, ignore file) that any saved analysis must match to be reused.

Sync: one branch, check, generate, deliver, publish

  1. Which branch? In branch mode, the repository's default branch, from any event; a sync anywhere else fails before the install (resolve-github-event.sh). A renamed default branch just continues, since entries are keyed by commit. In-place syncs the branch it runs on. force_full: true is ignored with a warning.

  2. Check (check-analysis-branch.sh). Before anything is installed, sync fails if codeboarding/baseline exists as a code branch, or a branch named codeboarding blocks creating it.

  3. Generate (sync-generate-baseline.sh:21-25) starts from the first of these that exists:

    1. the analysis-branch entry for this commit or an ancestor
    2. the committed .codeboarding/
    3. the saved artifact for this commit or an ancestor

    With none of them, or none compatible with the current configuration, it runs a full analysis.

  4. Deliver. The one branch point is deliver-sync.sh:42-48:

    • codeboarding_branch: save_to_codeboarding_baseline adds one fast-forward commit on the orphan branch.
    • in_place: commit .codeboarding/ and push it to the synced branch.
  5. Publish the result as an artifact named for the analyzed commit, so reviews that fork there reuse it.

Scenario What happens
First sync, default settings codeboarding/baseline is created as an orphan branch with one commit
Next push Continues from the branch entry (incremental) and adds one commit
codeboarding_analysis_location: in_place Continues from the committed .codeboarding/ and pushes the commit to the synced branch
A sync on a branch other than the default (manual run, second push branch) Fails before the install, naming the default branch
Default branch renamed, e.g. master → main Continues incrementally
The commit being synced deleted .codeboarding/ (the migration commit) Continues from the parent's copy
codeboarding/baseline exists as a code branch, or a branch named codeboarding exists Fails before the install
Synced branch still has an old committed .codeboarding/ Warning: it no longer updates and can be deleted
Nothing changed No commit
Synced branch moved during the analysis Branch mode: saved for the commit it analyzed, unless the branch already holds a newer commit's. In-place: dropped; the newer run saves it
Branch rule blocks the push Fails, naming the bypass to add
Analysis branch unreadable (token, network) Retried once, then fails

Review: event, base, head, publish, comment

  1. Against what? (resolve-github-event.sh:38-120) pull_request runs directly; /codeboarding only from collaborators; a fork PR only via /codeboarding. The comparison is against the merge base, never the base branch tip; the protected test enforces this.

  2. Base analysis (review-generate-baseline.sh:27-55) uses the first of these that exists:

    1. the published artifact for exactly this merge base, used as is
    2. the analysis-branch entry for it or an ancestor
    3. the .codeboarding/ committed at the merge base
    4. the nearest ancestor's artifact
    5. nothing, so a full analysis

    Options 2–4 are caught up to the merge base incrementally.

  3. Head analysis (review-analyze-head.sh:12-25) continues from this PR's last analysis only if that analysis grew from the same base analysis; otherwise it starts from the base. Two engine runs over one commit can name components differently, so mixing lineages would report changes nobody made.

  4. Publish the head analysis for the next push. The base analysis is published only when this run built a new one, and never for a fork PR.

  5. Render the diagram and post the sticky comment. On failure, the comment carries the reason.

Scenario Base comes from Head
First PR after a sync the artifact sync published (exact) incremental from the base
Second push to the same PR the same artifact continues from the PR's last analysis
Merge base a few commits after the last sync analysis-branch entry, caught up incremental from the base
Repo that never ran sync full analysis at the merge base incremental from the base
in_place .codeboarding/ committed at the merge base incremental from the base
Fork PR built fresh; nothing from the fork is saved for later runs from the base
Merge-base lookup fails the base tip, and the comment says so from the base

Input API

One new input: codeboarding_analysis_location. It sets where the synced branch's analysis lives; sync saves it there and review reads it from there.

  • codeboarding_branch (default): on codeboarding/baseline, which the webview also reads, for the default branch. The code is never written.
  • in_place: committed as .codeboarding/ on the synced branch. The README shows that setup with its paths-ignore.

In branch mode the synced branch is the repository's default branch; in-place, it is the branch the push trigger lists.

Removed:

  • workflow_dispatch in the sync examples and our own workflow: a manual run could sync any branch.
  • The pull_request delivery (rolling codeboarding/sync PR).

synced_branch, save_baseline_to and baseline_branch only ever existed on this branch.

The released target_branch, sync_strategy and force_full are handled only by old_api_migrator.sh:

Old input Becomes
sync_strategy: push codeboarding_analysis_location: in_place
sync_strategy: pull_request codeboarding_analysis_location: codeboarding_branch, plus a note to close the codeboarding/sync PR
target_branch naming the branch the run is on no effect
target_branch naming another branch fails: "list it under on: push: branches: instead"
force_full: true no effect, with a warning (false, which old templates send on every push, is silent)
sync_strategy agreeing with a set codeboarding_analysis_location that location, with a warning
sync_strategy disagreeing with a set location, or an unknown value fails

Every warning and failure links to the README's moving an existing setup, which has the steps and a prompt to paste into a coding agent.

Default change for current users: sync used to commit to the synced branch by default and now saves to codeboarding/baseline, and in that mode syncs the default branch only. Until the first sync creates that branch, reviews keep reading the old committed .codeboarding/, and sync warns that the old copy no longer updates. This ships as feat: on the moving v1 tag; AGENTS.md records it as the second exception, next to the credentials change.

Failures

These fail instead of silently running a full analysis, and record a reason:

  • an analysis branch that can't be read, after one retry (a branch that doesn't exist yet is a normal miss)
  • an entry that can't be fetched or extracted
  • an unreadable engine version
  • an invalid codeboarding_analysis_location
  • a sync in branch mode on a branch other than the default
  • a code branch named codeboarding/baseline, or a branch named codeboarding
  • a sync whose install, analysis or cache-key step fails (08d55da; install-sync.sh's errors used to go to stdout, which is redirected to a file, so they never reached the log)

A review shows the reason in its comment. A sync shows it as a Failed: line in the job summary, since a push has no comment. Input errors are not yet posted on the PR; they show in the log.

Other behaviour changes

  • In branch mode, a result is no longer dropped because the synced branch moved during the analysis: entries are keyed by commit, so it is saved unless the branch already holds a newer commit's.
  • The baseline branch now outranks a baseline committed on the branch: only its entries are pinned to the configuration, so leftover committed files no longer win.
  • When Core refuses a seed that passed our checks, the base is analyzed in full instead of retrying the next source. In practice only a stale committed baseline right after an engine bump hits this.
  • base_analysis_method / base_analysis_reason are gone from the review artifact, with the commit archaeology that produced them. Telemetry answers it: no -base engine run means reused.
  • The progress comment is two fixed edits (building base, analysing PR) instead of a per-minute background ticker.
  • Sync no longer deletes v1-generated Markdown, and reads Core's artifact manifest without the fallback for engines older than the pin. 0.14.5 ships constants.PERSISTED_ANALYSIS_ARTIFACT_FILENAMES.
  • docs/baseline-branch-ruleset.json lets only the CodeBoarding app write codeboarding/baseline, which only works with the app's token, and adopters can't mint that token. The README therefore points default-token setups at baseline-branch-ruleset-actions.json, which is documented as stopping manual pushes only.

Protected test

tests/test_merge_base_contract.py now calls resolve-github-event.sh and the two review steps instead of guard.sh / analyze.sh. Its assertions are unchanged. This edit was explicitly approved by a human in the authoring session.

Testing

256 unit tests pass locally (Python 3.12, codeboarding==0.14.5). They cover:

  • the migrator mappings and failures
  • in_place in sync and review
  • the default-branch check, and a renamed default branch
  • the checks for a code branch with that name, and for a codeboarding branch blocking it
  • saving after the synced branch moved, and never replacing a newer analysis
  • continuing from the parent's .codeboarding/ after the migration commit deleted it
  • force_full (true warns, false is silent) and an agreeing sync_strategy
  • an unreadable analysis branch failing with its reason
  • a single failed lookup being retried
  • the stale-copy warning
  • the sync summary's failure line, and install-sync.sh's failure reason

shellcheck -x now covers scripts/action/*.sh in CI and is clean locally (0.11); actionlint runs in CI only.

Our own workflow

codeboarding-sync.yml is folded into codeboarding.yml as a sync job (17cd1e8), matching the README single-workflow example.

  • Concurrency stays workflow-level, so closing a PR still cancels its review.
  • The App-token block that both jobs repeated is now .github/actions/app-token.
  • The workflow is named CodeBoarding (was CodeBoarding review / CodeBoarding sync); the job names review and sync are unchanged.
  • This repository keeps its analysis in place on main (codeboarding_analysis_location: in_place in both jobs).
  • Sync runs on pushes to main only.

🤖 Generated with Claude Code

Svilen-Stefanov and others added 7 commits October 7, 2026 17:05
A review's base comes from a saved artifact, a baseline committed at the
merge base, or a full analysis in this run, and nothing said which. The
slow path is the last one, and a reader had no way to tell why a run took
ten minutes instead of two.

analyze.sh now records base_source (saved, committed, computed), base_reason
(no_baseline, incompatible), base_from_sha, catchup_commits, base_seconds and
head_seconds. They go into the review metadata.json as strings, onto the
comment's machine-readable marker, and into one "Base:" line in the comment.
While a base is computed, the sticky progress comment is rewritten into two
steps with the elapsed time and the reason, never an estimate.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review follow-ups on the base provenance:

- code_paths_changed diffs against the first parent, so a --no-ff merge on
  the first-parent chain counts as the change it brought in; diff-tree
  printed nothing for merges and catch-up read 0. .gitattributes is not
  code either.
- base_from_sha for a committed baseline comes only from a commit sync
  made (its bot committer, nothing but .codeboarding/): pushed directly,
  or on the second-parent side of a merged sync pull request. Anything
  else (a squash, a hand edit) leaves it empty rather than guessing.
- An empty sha or count is unknown, not 0: the comment then says
  "saved diagram of <branch>, caught up" without a sha, and an ancestor
  never reads as "saved".
- The progress ticker stops through a stop file and is waited for, and
  post-progress.sh checks the file right before editing, so a stale
  "running" edit cannot land after step 2.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review feedback: base_source/base_reason/base_from_sha/catchup_commits used
"base" for too many things. A review compares the base commit's analysis with
the head's; what matters is how the base analysis was obtained.

- base_analysis_method: reused | incremental | full (replaces base_source).
  A committed baseline with nothing to catch up is reused.
- base_analysis_reason: the method in words; the starting commit and the
  number of commits caught up are detail in it, not fields of their own.
- base_seconds / head_seconds unchanged.
- The comment's Base: line moves under the diagram, above the run links,
  and the marker carries base_analysis_method instead of base/base_reason.
- Drops the too_far_behind and ancestor wording, which belong to the
  ancestor lookup in the next PR.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With no saved base for the merge base and no usable baseline committed
there, a review analyzed the merge base from scratch, even when a saved
analysis of a commit a few steps below it was sitting in the artifact store.

find-ancestor-base.sh walks the merge base's first-parent history up to
100 commits, deepening the shallow checkout, then pages through the
artifact listing newest first, keeping base analyses for this
configuration that a run on the repository's own code produced (the same
provenance rule as fetch-state.sh). It stops at the first page holding a
walked commit (50 pages at most; one or two calls in practice, and only on
a run that would otherwise analyze from scratch). The nearest hit seeds an
incremental catch-up to the merge base, which is then published under the
merge base's own name. Order: exact artifact, committed at the merge base,
nearest ancestor, full. Seeding keeps the merge base's own
.codeboardingignore and health configuration.

Reported as base_analysis_method=incremental, the reason naming the
ancestor and the commits caught up. A saved analysis of the merge base
under another configuration makes a full run's reason "incompatible";
anything else is "no usable analysis was available". There is no separate
"too far behind" reason: proving it took compare API calls that only
changed the wording.

Sync uses the same lookup when the branch has no usable committed
baseline, so the first sync after the setup pull request merges catches
up from the base that pull request's review saved. FORCE_FULL is now
lowercased with tr, which also runs on the bash 3.2 macOS ships.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sync_strategy: branch saves the analysis to an orphan branch in the same
repository, codeboarding/analysis unless analysis_branch names another, one
fast-forward commit per sync, and never writes to the target branch. Each
commit carries CodeBoarding-Source and CodeBoarding-Config trailers, also
recorded in .codeboarding/source.json.

Reviews read their base from the branch after an exact artifact and a
baseline committed at the merge base: an entry for the merge base is reused,
an entry for an ancestor is caught up. Only entries made under this run's
configuration count, so a different engine version or model is never reused
as is. Sync continues from the tip under the same rule, replacing the
generated state wholesale and keeping only the checkout's user config.

Delivery refuses an existing branch that is not an analysis branch (no
trailer, or anything besides .codeboarding/), builds on the tip it fetched
rather than the one ls-remote saw, and the guard rejects an analysis_branch
git cannot use before anything runs. The importable ruleset now restricts
creating and updating the branch to its bypass actor, since sync and review
load a pickle from it.

target_branch's description now says it is the code branch sync analyzes,
written only by push and pull_request. The README carries a prompt for
moving an existing setup.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…emetry

target_branch becomes synced_branch, sync_strategy becomes save_baseline_to
(synced_branch | pull_request | baseline_branch), and the unreleased
analysis_branch becomes baseline_branch with default codeboarding/baseline.
The released names keep working through old_api_migrator.sh, the only reader
of deprecated inputs.

The base_analysis_method output and the review comment's base line and
timers are removed. Engine runs carry CODEBOARDING_RUN_ID tagged base, head
or sync, so the engine's own telemetry shows how each base was obtained.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…one commit at a time

analyze.sh now only analyzes one checkout into a state directory. Where an
analysis to continue from comes from lives in seed-sources.sh, and the
baseline branch is read and written in one place, codeboarding-baseline.sh.
The review runs as two steps, review-generate-baseline.sh and
review-analyze-head.sh; sync runs sync-generate-baseline.sh.

The baseline branch is now consulted before a baseline committed on the
branch, since only its entries are pinned to the configuration. The commit
archaeology that existed only to report how a base was obtained is gone, with
base_analysis_method and base_analysis_reason: engine runs carry their role
in CODEBOARDING_RUN_ID instead. The progress comment is two fixed edits
rather than a background ticker.

guard.sh, state-names.sh and post-progress.sh are renamed to
resolve-github-event.sh, resolve-cache-keys.sh and update-review-progress.sh.
tests/test_merge_base_contract.py follows the new script paths, with
explicit consent; its assertions are unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@codeboarding-review

codeboarding-review Bot commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

CodeBoarding review

Status: 0 changed components (no analysed file changed)

See the full change in CodeBoarding.

graph LR
    n_action_scripts["action_scripts"]
    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;
Loading

download artifacts · run 38172809419

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 10, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-11T20:41:34.539243Z ee4a205 New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 608b37dd48

ℹ️ 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".

Comment thread action.yml Outdated
Comment thread scripts/action/old_api_migrator.sh Outdated
ivanmilevtues and others added 3 commits October 10, 2026 03:41
codeboarding-sync.yml folds into codeboarding.yml as a second job, the way the
README now recommends. Concurrency stays workflow-level so a closed pull request
still cancels its running review, and syncs still share one group. The App
token block both jobs repeated is now the local action .github/actions/app-token.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
with-auth.sh removes the credentials once its command finishes, so splitting
the review into a baseline step and a head step left the head step with none.
The baseline step now keeps them (KEEP_AUTH), the head step removes them, and
a final always() step clears them if a failure skipped the head step.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sync catches up from a saved ancestor analysis, which needs actions: read to
list artifacts. Without it the lookup fails quietly and sync analyzes in full.
The README's sync setups and this repository's sync job now grant it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7776d203b8

ℹ️ 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".

Comment thread tests/test_merge_base_contract.py
@ivanmilevtues ivanmilevtues changed the title refactor: split the review into baseline and head steps, and analyze one commit at a time feat: save the baseline to a branch of its own, and generate the review's base in a step of its own Oct 10, 2026

@Svilen-Stefanov Svilen-Stefanov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this generally makes sense. It seems like we would still support 1 and 2 WF setups - idk if you left this for current users but technically it would be nicer to just have 1 WF file. We need to decide how to show this to users though after the action is released.

Comment thread action.yml Outdated
Comment thread action.yml Outdated
…tead of degrading silently

One input, codeboarding_analysis_branch (default codeboarding/baseline),
replaces synced_branch, save_baseline_to and baseline_branch. Sync analyzes
the branch it runs on; naming that branch commits .codeboarding/ there
instead of to the analysis branch. Rolling sync pull requests are gone.

The released target_branch and sync_strategy inputs are rewritten into the
new input by old_api_migrator.sh, or fail with a link to the migration
steps; no other script knows they exist.

Failures that used to fall back to a full analysis without saying so now
fail: an analysis branch that cannot be read, an entry that cannot be
fetched, an unreadable engine version, an empty analysis branch name, and a
sync run on the analysis branch itself.

Sync no longer deletes v1-generated Markdown, and reads Core's artifact
manifest without the fallback for engines older than the pinned one.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ivanmilevtues

Copy link
Copy Markdown
Member Author

@Svilen-Stefanov on the review summary (one vs two workflow files): the action does not depend on either. Review and sync are just mode, so how they are orchestrated is up to whoever integrates it. The README shows both, and the one-file version needs no shared settings any more, since both jobs use the default codeboarding_analysis_branch. Our own repo runs the one-file setup.

How we present this to users after the release is still open. What ships with it: old inputs warn or fail with a link to moving an existing setup, which has the steps and a prompt to paste into a coding agent. If the web platform gets a migration page, we can point the links there instead.

The PR description now has an overview of how sync and review are orchestrated, with the scenarios for each.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@Svilen-Stefanov Svilen-Stefanov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this reads a lot cleaner now with just codeboarding_analysis_branch 👍 Moving the default to codeboarding/baseline is fine with me.

A few things I'd like to sort out before merging, mainly: sync should fail loudly when it runs on a branch it isn't meant for, and the new failures should actually say what went wrong.

Comment thread scripts/action/resolve-github-event.sh
Comment thread scripts/action/codeboarding-baseline.sh Outdated
Comment thread scripts/action/old_api_migrator.sh Outdated
Comment thread scripts/action/deliver-sync.sh Outdated
Comment thread README.md Outdated
Comment thread scripts/action/codeboarding-baseline.sh Outdated
Comment thread scripts/action/codeboarding-baseline.sh Outdated
Comment thread docs/baseline-branch-ruleset.json Outdated
… per repository

codeboarding_analysis_location (codeboarding_branch | in_place) replaces
codeboarding_analysis_branch: the only choice is where the analysis of the
synced branch lives, and a free branch name only added ways to name the wrong
one. The analysis branch is always codeboarding/baseline.

The push trigger picks the synced branch, and it lists exactly one. Each sync
records CodeBoarding-Branch on codeboarding/baseline, and a new step before the
checkout and the engine install refuses a sync from another branch, or a code
branch already named codeboarding/baseline, instead of failing after a full
analysis. workflow_dispatch and force_full are gone from sync: a manual run
could sync any branch, and a full analysis is now only for a missing or
incompatible saved one.

Failures on the analysis branch record a reason the review comment and the
sync job summary show; its lookup is retried once. Sync warns while the synced
branch still holds an old committed analysis. The recommended ruleset lets only
the CodeBoarding app write codeboarding/baseline.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 381a0c4137

ℹ️ 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".

Comment thread scripts/action/codeboarding-baseline.sh Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated

@Svilen-Stefanov Svilen-Stefanov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, most of it look good now 👍 I'm ok with the one-owner-branch approach instead of pinning to the default branch, it's more flexible. A few gaps left around ownership, plus force_full and the README. Nothing big.

Comment thread scripts/action/check-analysis-branch.sh Outdated
Comment thread scripts/action/check-analysis-branch.sh Outdated
Comment thread scripts/action/old_api_migrator.sh
Comment thread README.md
Comment thread scripts/action/resolve-github-event.sh
Comment thread scripts/action/codeboarding-baseline.sh Outdated
Comment thread scripts/action/codeboarding-baseline.sh Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
Sync saves to codeboarding/baseline-<branch>, and a review reads the one of
its base branch. There is no ownership record left to go stale: a renamed
branch, a branch switch or a manual run elsewhere starts its own analysis
branch instead of locking the synced branch out.

Also: force_full is accepted again and ignored with a warning; sync failures
in install, analysis and cache-key steps state their reason in the job
summary; the README points default-token setups at the GitHub Actions
ruleset and documents the one synced branch.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ivanmilevtues ivanmilevtues changed the title feat: save the baseline to a branch of its own, and generate the review's base in a step of its own feat: save each branch's analysis to a branch of its own, and generate the review's base in a step of its own Oct 11, 2026
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@Svilen-Stefanov Svilen-Stefanov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick turnaround 👍

One analysis branch per code branch. I'm not sure about this one. In someone else's repo it keeps adding branches nobody cleans up (a manual run, a rename, a second push branch), and the rulesets block deleting them. A single codeboarding/baseline works fine even if the synced branch changes, since every entry is keyed by its commit. The only thing we need is to stop syncs from the wrong branch. Options:

  1. Default branch + optional override (my preference). Sync runs on the repo's default branch, or on an optional synced_branch input (e.g. dev). Anything else fails in seconds. One branch, nothing stored that can go stale, and a master -> main rename just continues incrementally.
  2. First sync owns it (what 381a0c4 had), plus: only a push can create the branch, and a sync can take over when the owner branch no longer exists.
  3. Later, as a spike: a hidden ref like refs/codeboarding/baseline instead of a branch. Not in the branch list, not in clones, doesn't trigger other workflows. But no rulesets, so needs a test first.

Whichever we pick, the webview (https://app.codeboarding.org/CodeBoarding/CodeBoarding-webview/pull/210) still reads codeboarding/baseline, so we need the same name on both sides plus a test that pins it.

Rest are smaller things from another review pass. One that didn't land on a changed line: CI only shellchecks scripts/run_local.sh (test.yml:77), not scripts/action/*.sh, but the description says it runs in CI.

Comment thread scripts/action/resolve-github-event.sh Outdated
# committed in place, on the code branch itself. Named after the code branch, so
# each code branch has its own and a sync can never write into another's.
analysis_branch_of() {
[ "${ANALYSIS_LOCATION:-}" = codeboarding_branch ] && [ -n "$1" ] || return 0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the part I'd change, see the summary. E.g. someone runs sync by hand from feature-x: today that creates codeboarding/baseline-feature-x with a full analysis and the branch stays forever. With option 1 it just fails with "sync only runs on main".

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, went with option 1 without the override (f1ebc0c). Branch mode is back to a single codeboarding/baseline, and a sync on any branch but the repository's default fails in seconds, before the install. A manual run from feature-x now fails instead of creating a branch, and a master→main rename just continues, since entries are keyed by commit. I left out synced_branch until someone needs a non-default sync branch. In-place has no restriction, since it commits to the branch it runs on. The test now pins the name codeboarding/baseline, with a note that the webview reads it.

Comment thread scripts/action/codeboarding-baseline.sh Outdated
# built on top of once. A second move means a newer run is handling it.
for _ in 1 2; do
git fetch -q "$REMOTE" "$SYNCED_BRANCH"
if [ "$(git rev-parse FETCH_HEAD)" != "$BASE_SHA" ]; then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In branch mode we don't need to drop the result when main moved, that rule is for in-place. E.g. a repo that merges every 5 min with a 10 min sync never saves anything, so the platform never gets a diagram. Can we save here when the new commit descends from the tip's CodeBoarding-Source?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f1ebc0c. Branch mode no longer drops the result when the synced branch moved. It saves the analysis for the commit it analyzed, and skips only when the tip's CodeBoarding-Source already descends from that commit, so an older analysis never replaces a newer one. Tests cover both cases. In-place keeps the drop rule.

fi

if ! from_codeboarding_baseline "${REPOSITORY:-}" "$head_sha" "$state" "$CHECKOUT_DIR" &&
! from_committed_baseline "$CHECKOUT_DIR" "$state" &&

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just checking the order in branch mode: when the analysis branch has no matching entry, the frozen .codeboarding/ left on main wins over the artifacts every sync publishes. So each run catches up from an older and older point. Should artifacts come before the committed copy here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving the order as it is. Each setup checks its own source first (branch mode the branch entry, in-place the committed copy), then the rest. A stale .codeboarding/ left on main with no branch entry is the migration edge case, and the next item covers that, so we'd rather keep the order simple.

# commit it describes is not recorded, so it is always caught up.
from_committed_baseline() {
local checkout="$1" state="$2"
[ -f "$checkout/.codeboarding/analysis.json" ] || return 1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to the webview update PR: it deletes .codeboarding/ in the same commit that switches storage, so the first sync finds nothing here and runs in full. Could this fall back to .codeboarding/ at the previous commit (first parent) when it's missing at HEAD? Then one PR is fine without the extra cost.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in f1ebc0c. When the commit being synced deleted .codeboarding/, sync deepens the checkout by one and continues from the parent's copy (depth-checked like the others), so the webview PR's migration commit doesn't cost a full run. There's a test for it.

Comment thread scripts/action/old_api_migrator.sh Outdated
fi

# Forcing a fresh analysis is a manual job now: run the CodeBoarding CLI and commit its result.
[ -z "${OLD_FORCE_FULL:-}" ] ||

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Old templates pass force_full: ${{ inputs.force_full || false }}, which arrives as "false" on every push, so every sync warns. Can we treat false like unset? Also "run the CLI locally" doesn't really help with branch storage, since the branch wins over a committed copy. Maybe that's a reason to keep a real way to force a fresh run?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch on the template. false (and unset) is now silent, and only force_full: true warns (f1ebc0c). Agreed the CLI hint didn't hold in branch mode, so I dropped it. We'll leave a real force path out for now and add one if someone needs it.

Comment thread scripts/action/check-analysis-branch.sh Outdated

status=0
baseline_branch_exists "$REPOSITORY" || status=$?
# The first sync creates it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edge case: if the repo has a branch called codeboarding, git can't create codeboarding/baseline... next to it. This check says "doesn't exist yet", we run a full analysis, and then the push fails blaming a branch rule. Maybe check for that here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in f1ebc0c. When codeboarding/baseline doesn't exist yet, the pre-install check also looks for a branch named codeboarding and fails naming it, instead of running the analysis and then blaming a branch rule. There's a test for it.

Comment thread README.md
The first run, `force_full: true`, or an incompatible baseline causes a full analysis. Otherwise sync asks Core for an incremental update. If the generated state is unchanged, no commit is created. If the target advances while analysis is running, the stale result is not rebased onto code it did not analyze; the newer push run is allowed to produce the current baseline.
`main` is only read. Each sync adds one commit to `codeboarding/baseline-main` in the same repository, an orphan branch that shares no history with `main`. It holds the `.codeboarding/` files listed above, plus `.codeboarding/source.json` naming the commit they describe and the configuration that made them; the commit message carries both as `CodeBoarding-Source:` and `CodeBoarding-Config:` trailers. Pushes only ever fast-forward, and no code branch is ever written, so the `push` trigger needs no `paths-ignore` and a protected `main` needs no exception. Reviews read their base from the branch, and the web platform reads the latest diagram from it. The [analysis branch section](docs/COMMIT_STRATEGY.md#the-baseline-branch) covers what happens if the branch is deleted, and a ruleset you should import to protect it: sync and review load a pickle from it.

The branch push itself needs nothing beyond `contents: write`: the first sync creates the branch with an ordinary push. If you import the ruleset that protects it, import the [GitHub Actions variant](docs/baseline-branch-ruleset-actions.json): the example pushes with the default `github.token`, which the CodeBoarding app ruleset refuses, so the sync would fail at the push after a full analysis.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more for this section: pushes with an App token or PAT trigger the user's own on: push workflows, so their CI runs on the analysis branch (no code) and fails. Maybe document branches-ignore: ['codeboarding/**']?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to the README (f1ebc0c): when sync pushes with an App token or a PAT, exclude the analysis branch from your own push workflows with branches-ignore: ['codeboarding/**'].

"type": "deletion"
},
{
"type": "non_fast_forward"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder about growth: every push adds a commit with a full pickle, git clone pulls it for everyone, and this rule means we can never squash it later without every adopter changing their ruleset. Should we decide a pruning policy before people import these?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skipping this for now. As far as I know, the bypass actor isn't held to non_fast_forward, so if growth ever becomes a problem, sync itself could prune by force-pushing and adopters wouldn't need to change their ruleset. I'd decide on a policy when we see real sizes.

Comment thread README.md Outdated
It does **not** render or commit architecture Markdown, and leaves every other file alone, including user-authored CodeBoarding configuration such as `health/health_config.json` and `health/.healthignore`.

Create `.github/workflows/codeboarding-sync.yml`:
Sync keeps the analysis of exactly one branch current: the one branch you list under `on: push: branches:`. It is started by pushes only, so it never runs anywhere else. `codeboarding_analysis_location` decides where that analysis is stored: by default on an analysis branch named after it, `codeboarding/baseline-main` for `main`. Create `.github/workflows/codeboarding-sync.yml`:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "started by pushes only", but resolve-github-event.sh still accepts workflow_dispatch and schedule for sync. Either reject them there or reword this.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworded (f1ebc0c). With the default-branch check, the event no longer matters: the README says sync keeps the default branch's analysis, and a sync on any other branch, from any event, fails before the install.

Comment thread scripts/action/old_api_migrator.sh Outdated
if [ -n "${OLD_SYNC_STRATEGY:-}" ]; then
# codeboarding_analysis_location has a default, so only a value other than it is a conflict.
[ "$location" = "$DEFAULT_LOCATION" ] ||
fail "Set codeboarding_analysis_location only; sync_strategy is the input it replaces."

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: sync_strategy: push together with codeboarding_analysis_location: in_place fails here even though they agree. Could happen halfway through a migration, maybe just warn?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done (f1ebc0c). When the two agree it only warns, and it fails only when they disagree, e.g. pull_request with in_place.

…analyses the branch moved past

Back to a single analysis branch, which the webview reads: in branch mode a
sync on any branch but the default fails before anything is installed, and a
renamed default branch just continues, since entries are keyed by commit.

Also:
- branch mode saves an analysis even when the synced branch moved during it,
  unless the analysis branch already holds a newer commit's
- the first sync after .codeboarding/ was deleted continues from the parent's copy
- a branch named codeboarding, which blocks creating the analysis branch, fails early
- force_full: false is silent; sync_strategy agreeing with the location only warns
- README documents branches-ignore for App-token pushes
- CI shellchecks scripts/action/*.sh

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ivanmilevtues

Copy link
Copy Markdown
Member Author

@Svilen-Stefanov thanks for the pass, all threads are answered. On the shellcheck point from the summary: CI now runs shellcheck -x on scripts/run_local.sh and scripts/action/*.sh (f1ebc0c). The sourced libraries got shell and source= directives, and the few findings are fixed. The main change is option 1: one codeboarding/baseline for the default branch, matching the webview.

@ivanmilevtues ivanmilevtues changed the title feat: save each branch's analysis to a branch of its own, and generate the review's base in a step of its own feat: save the default branch's analysis to a branch of its own, and generate the review's base in a step of its own Oct 11, 2026
The README built the setup as a review file and a sync file, and offered one
file at the end. One file with a review and a sync job is now the setup, the
review and sync sections describe their jobs, and separate files are an older
setup to fold in: the webview flags them and opens that pull request. The
migration prompt deletes only the six files sync writes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ee4a205ee3

ℹ️ 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".

baseline_branch_exists "$REPOSITORY" || status=$?
if [ "$status" = 2 ]; then
# The first sync creates it, which git refuses next to a branch named like its parent directory.
blocking="${BASELINE_BRANCH%/*}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject descendant refs that block the baseline branch

When the repository already has a branch such as codeboarding/baseline/foo, the exact-name lookup reports codeboarding/baseline as absent and this check only probes its parent, codeboarding. Git cannot create a ref that is the prefix of the existing descendant, so preflight succeeds, the full analysis runs, and delivery eventually fails with a misleading branch-rules error. Also check for refs below refs/heads/$BASELINE_BRANCH/ before allowing the run.

Useful? React with 👍 / 👎.

Comment on lines +163 to +164
git fetch -q --depth="$(( CATCHUP_BOUND + 1 ))" "$REMOTE" "$SYNCED_BRANCH"
if git merge-base --is-ancestor "$BASE_SHA" "$tip_source" 2>/dev/null; then

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Detect newer sources beyond the catch-up window

When the synced branch advances by more than the default 100-commit catch-up bound while this run analyzes an older commit, this shallow fetch omits BASE_SHA; if another sync has already saved the newer tip, git merge-base --is-ancestor cannot establish the relationship and the code falls through to append the stale analysis on top of it. Because the branch tip is the diagram readers consume, this temporarily replaces the current baseline with an older one; deepen on demand or use an unbounded ancestry check before appending.

AGENTS.md reference: AGENTS.md:L8-L12

Useful? React with 👍 / 👎.

Comment on lines +62 to +63
ancestor="$(nearest)"
[ -z "$ancestor" ] || break

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Search later artifact pages for a nearer ancestor

When a farther ancestor's base artifact was created or renewed recently enough to appear on the first page, but a nearer ancestor has an older artifact on a later page, this early break selects the farther commit without ever seeing the nearer one. Artifact creation time is unrelated to ancestry distance, so the run can needlessly reanalyze many additional commits and consume substantially more LLM time and quota; continue paging while tracking the nearest saved commit rather than stopping at the first page with any match.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants