Skip to content

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

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

ivanmilevtues wants to merge 13 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.

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 (:14-15).
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 keeping this branch's analysis.
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? The one branch the workflow's push trigger lists. Sync is started by pushes only, so it never runs anywhere else (resolve-github-event.sh:16-36). There is no manual trigger and no force_full.

  2. Check (check-analysis-branch.sh). Each commit on codeboarding/baseline records CodeBoarding-Branch: <branch>. Before anything is installed, sync fails if a different branch already keeps its analysis there, or if codeboarding/baseline exists as a code branch.

  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, which records the synced branch
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
Trigger lists a second branch, e.g. [main, dev] A push to dev fails before the install: "codeboarding/baseline keeps the analysis of main, and this sync runs on dev"
codeboarding/baseline exists as a code branch 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 Result dropped; the newer run's sync 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. The code is never written.
  • in_place: committed as .codeboarding/ on the synced branch. The README shows that setup with its paths-ignore.

The push trigger picks the synced branch, and lists exactly one.

Removed:

  • force_full: a full analysis now happens only when no compatible saved analysis exists.
  • 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 and sync_strategy 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"
sync_strategy with a non-default 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. A workflow still passing force_full gets GitHub's "Unexpected input" warning and runs a normal sync.

Default change for current users: sync used to commit to the synced branch by default and now saves to codeboarding/baseline. 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 from a second branch
  • a code branch named codeboarding/baseline

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

  • 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.
  • The recommended ruleset, docs/baseline-branch-ruleset.json, lets only the CodeBoarding app write codeboarding/baseline. The GitHub Actions variant, baseline-branch-ruleset-actions.json, 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

246 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 one-branch check (first sync, same branch, second branch, a code branch with the name)
  • an unreadable analysis branch failing with its reason
  • a single failed lookup being retried
  • the stale-copy warning
  • the sync summary's failure line

shellcheck / actionlint run 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 38103828993

@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-11T02:08:52.132711Z 381a0c4 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.

if [ "$SYNC_STRATEGY" = pull_request ]; then
sync_branch_start_sha="$(gh api "repos/$REPOSITORY/branches/codeboarding%2Fsync" --jq '.commit.sha' 2>/dev/null || true)"
fi
synced_branch="$REF_NAME"

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 sync should only ever run on the branch that keeps the analysis long term, and fail loudly anywhere else. Right now it syncs whatever branch the run is on, so:

  • "Run workflow" from feature-x with the default storage adds feature-x's diagram to codeboarding/baseline, and the platform shows it as the latest one
  • with codeboarding_analysis_branch: main and a run on develop, we do a full analysis and only fail at the push

Can't we just say sync runs on the repo's default branch (github.event.repository.default_branch) and fail before the install otherwise? For in-branch storage that also means codeboarding_analysis_branch has to be the default branch. If someone really needs another core branch we can add that later.

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.

Agree with the problem, but I would rather not pin sync to the repo's default branch. Some repos keep their baseline on dev, and the person should choose; our UI already does when it generates the workflow.

So the push trigger is the choice, and it lists exactly one branch. Your feature-x case came from the manual trigger: workflow_dispatch ignores the push filter. It only existed for the force_full button, which nobody used, so both are gone (381a0c4).

To enforce "one branch", each sync now records CodeBoarding-Branch: <branch> on codeboarding/baseline. A new step runs before the checkout and the engine install and fails if another branch tries to sync: "codeboarding/baseline keeps the analysis of main, and this sync runs on dev."

I also replaced the branch-name input with codeboarding_analysis_location: codeboarding_branch | in_place. The only real choice is where the analysis lives, and a free branch name is how your main + develop case happened. With in_place the analysis always goes to the branch being synced, so that case cannot happen any more.

Comment thread scripts/action/codeboarding-baseline.sh Outdated
# Building on any other branch would leave it holding nothing but .codeboarding/.
if [ -z "$(git log -1 --format='%(trailers:key=CodeBoarding-Source,valueonly)' "$tip" | tr -d '[:space:]')" ] ||
[ "$(git ls-tree --name-only "$tip")" != .codeboarding ]; then
echo "::error::$branch already exists and is not a CodeBoarding baseline branch, so sync will not write to it. Set codeboarding_analysis_branch to a branch name that is not in use, or to $SYNCED_BRANCH to commit the analysis there."

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.

Can we run this check before analyzing instead of at push time? With the default-branch rule this mostly goes away, but someone can still point codeboarding_analysis_branch at an existing code branch, and today we only find out after a full 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.

Done in 381a0c4. The same pre-install step refuses a codeboarding/baseline that already exists as a code branch, so nobody pays for a full analysis first. The push-time check stays for a branch created mid-run.

Comment thread scripts/action/old_api_migrator.sh Outdated
if [ "$MODE" = sync ]; then
if [ -n "${OLD_TARGET_BRANCH:-}" ]; then
[ "$OLD_TARGET_BRANCH" = "$REF_NAME" ] ||
fail "target_branch is no longer supported: sync analyzes the branch it runs on, which for this run is $REF_NAME, not $OLD_TARGET_BRANCH. Remove target_branch: $OLD_TARGET_BRANCH, and sync analyzes $REF_NAME."

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.

With sync only on the default branch this message can get simpler too. Right now it tells someone with target_branch: develop to remove it and analyze main instead, which quietly changes what they wanted. Maybe just say sync only runs on the default branch now and link the moving section?

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, it should not quietly change what they wanted. It now says: "target_branch is no longer supported: sync analyzes the branch its push trigger lists, and this run is on master. List develop under on: push: branches: instead, and remove target_branch." Plus the link to the migration section.

Comment thread scripts/action/deliver-sync.sh Outdated
}

source "$ACTION_PATH/scripts/action/codeboarding-baseline.sh"
[ -z "${BASELINE_BRANCH:-}" ] || save_to_codeboarding_baseline

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.

For repos that never set anything, the .codeboarding/ on main just stops updating after this and nobody tells them. We can't delete it from here (main is never written), but can we at least warn when we save to the separate branch and main still has generated files? Something like "main still has an old analysis in .codeboarding/, you can delete it (keep .codeboardingignore and the health config)". The webview update cleans it up for repos that go through it.
I think we generally should always check the codeboarding branch + other branches to let users know if they have .codeboarding somewhere else they probably don't want to but idk, we can do that later as well.

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 381a0c4. When sync saves to codeboarding/baseline and the synced branch still has a committed .codeboarding/analysis.json, it warns that the copy no longer updates and can be deleted, keeping .codeboardingignore and the health config. The explorer will clean it up for repos that go through it. Scanning other branches I have left for later, as you suggested.

Comment thread README.md Outdated
```

Generation is identical to direct push. Only delivery changes: the same commit is force-with-lease pushed to the machine-owned `codeboarding/sync` branch and one rolling PR is opened into `target_branch`. When there is no longer a generated diff, an obsolete rolling PR is closed.
Both jobs use the default `codeboarding_analysis_branch`, so review always reads the analysis where sync saves it. If you set it, set it in both jobs. Existing two-file setups keep working unchanged; this is only a different way to call the same action.

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: this isn't true anymore for sync users on the defaults, their analysis moves to codeboarding/baseline. Maybe say that here, and add the same exception note to AGENTS.md that the credentials change has?

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: two-file setups work the same way, except that a sync on the old defaults now saves to codeboarding/baseline, with a link to the migration section. AGENTS.md now notes this as a second feat:-not-feat!: exception, next to the credentials one.

Comment thread scripts/action/codeboarding-baseline.sh Outdated
case "$status" in
0) ;;
2) echo "::notice::$BASELINE_BRANCH does not exist yet; the first sync creates it."; return 1 ;;
*) echo "::error::Could not read $BASELINE_BRANCH from $repository. Check that github_token can read the repository."; exit 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.

These new exit 1s fail the review with an empty reason in the comment, since only Core sets failure_reason (action.yml:730). Can we write failure_reason here so the comment says what went wrong? Same for the migrator, which fails before mode is known, so there's no comment at all. And a failed sync has no surface besides the red check, maybe at least a job summary line?

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.

Partly, in 381a0c4:

  • Every failure in the analysis-branch code now records failure_reason, so the review comment says what went wrong.
  • A failed sync gets a Failed: line with the reason in the job summary. That covers the event check, the new pre-install check, the analysis and the push.

Migrator failures reaching the PR comment I have left out for now. It needs the run to keep going past the migrator to find the PR, and two more steps to report and then stop. For now they show in the log.

Comment thread scripts/action/codeboarding-baseline.sh Outdated
# A branch that does not exist yet is a miss: the first sync creates it. Any other
# failure to read it is an error, not a reason to quietly analyze from scratch.
auth="$(printf 'x-access-token:%s' "${GIT_TOKEN:-}" | base64 -w0)"
git -c "http.extraheader=AUTHORIZATION: basic $auth" ls-remote --exit-code \

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, a single network blip here now fails the whole review instead of falling back. Can we retry once, like the merge-base lookup in resolve-github-event.sh does?

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.

Yes, the lookup now retries once (2s) before failing, in reviews and in the pre-install check. A branch that does not exist is not retried; that is just the first sync.

Comment thread docs/baseline-branch-ruleset.json Outdated
],
"bypass_actors": [
{
"actor_id": 15368,

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 how much this protects. 15368 is GitHub Actions, so any workflow with contents: write (also one on a same-repo PR branch) can still push here and plant the pickle that sync loads. Should we recommend the app-only bypass as the default, and say clearly that this variant only stops people pushing by hand?

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. baseline-branch-ruleset.json is now the App-only ruleset: only the CodeBoarding app can write the branch, so sync has to push with the app token. The GitHub Actions variant moved to baseline-branch-ruleset-actions.json, and COMMIT_STRATEGY.md says plainly that it only stops manual pushes: any workflow with contents: write can still plant the pickle.

… 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 on lines +150 to +152
if [ -z "$(git log -1 --format='%(trailers:key=CodeBoarding-Source,valueonly)' "$tip" | tr -d '[:space:]')" ] ||
[ "$(git ls-tree --name-only "$tip")" != .codeboarding ]; then
baseline_fail "$branch already exists and is not a CodeBoarding baseline branch, so sync will not write to it. Rename that branch, or set codeboarding_analysis_location: in_place."

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 Enforce branch ownership in the delivery-time race

When two first-time syncs from different source branches overlap, both can pass the preflight while the baseline branch is absent. If the other run creates it during analysis, this delivery-time check validates only the source trailer and tree shape, so the losing run can append an entry for its own branch and make subsequent syncs for the intended branch fail the ownership check. Recheck the fetched tip's CodeBoarding-Branch against SYNCED_BRANCH here before creating the commit.

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

Useful? React with 👍 / 👎.

Comment thread README.md

No permission beyond `contents: write` is needed: the first sync creates the branch with an ordinary push, and records which branch it keeps the analysis of. A sync from any other branch then fails before anything is installed, naming both branches. To keep another branch's analysis instead, change the push trigger and delete `codeboarding/baseline`; the next sync creates it again.

To sync more than one branch, list them under `push: branches:`. Each run analyzes the branch that was pushed, and every pull request is compared with the analysis of its own merge base.

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 Remove the unsupported multi-branch sync instruction

When a user follows this instruction and lists multiple branches, the first sync makes codeboarding/baseline belong to its source branch, and check-analysis-branch.sh then rejects a sync from any other branch at lines 28–30. The action therefore cannot keep each listed branch's analysis current as promised here; document the one-branch restriction instead, or provide separate baseline storage per branch.

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

Useful? React with 👍 / 👎.

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` 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.

No permission beyond `contents: write` is needed: the first sync creates the branch with an ordinary push, and records which branch it keeps the analysis of. A sync from any other branch then fails before anything is installed, naming both branches. To keep another branch's analysis instead, change the push trigger and delete `codeboarding/baseline`; the next sync creates it again.

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 Retain the required OIDC permission in sync guidance

For the shown llm: hosted sync configuration, contents: write alone cannot run the action: _require_id_token rejects the configuration before analysis unless the job also grants id-token: write. This sentence contradicts the working example immediately above and can cause users who simplify its permissions to get a guaranteed missing_id_token failure; clarify that only the baseline push itself needs no permission beyond contents: write.

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