fix(engine): reframe cfg.cyclomatic remediation as a trade-off - #318
Merged
Merged
Conversation
`cfg.cyclomatic` is a whole-file sum: every function contributes +1 just by existing, so "extract helper functions" preserves the decisions and adds a function -- it can only push this number up. The advice was self-defeating. Name the levers that actually move a whole-file sum (collapse redundant decisions, or split the file) and surface the real trade-off against `ast.max_function_complexity`, which extraction does improve. Guidance wording only: no change to metric computation, gates, or thresholds. `cfg.cyclomatic` stays advisory (`gates_achieved: false`, issue #193). Closes #286 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jeremy-wayland
added a commit
that referenced
this pull request
Aug 9, 2026
* docs: document squash-merge + Unreleased changelog convention Records the repo git convention: main's history is a series of squash-merged PRs, each carrying its own CHANGELOG [Unreleased] entry. Release scope is decided after the work lands, and the release PR -- which renames [Unreleased] to a version heading -- comes last. Explicitly rules out long-lived release branches that accumulate feature PRs, superseding the v0.4.x stacked-PR-into-release-branch pattern. Opens the [Unreleased] section and backfills entries for the four v0.6.0 fixes already in review (#317, #318, #319, #320), which were authored before this convention existed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: scope PRs to one issue cluster, add stacked-PR rules Stacking is for work review surfaced -- a required fix to a PR under review, or a distinct issue found while reviewing it -- and stacks onto the PR branch, never onto a release branch. Documents the replant step (git rebase --onto main) that stacked-squash divergence requires, and notes the cheaper default: if the parent has not merged and the fix belongs to its issue, push another commit to the parent instead of stacking. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: tighten PR scope, stacking test, and replant procedure Scope a PR by coherent outcome -- reviewed, reverted, and explained as one unit -- rather than by issue count, and make the PR title the permanent squash subject. Replace the "review surfaced it" test for stacking with the stricter "child structurally depends on an unmerged parent"; discovery during review is not itself a reason to stack. Bound stacks to two or three, require the ordered stack in every description, and review bottom-up. Correct the replant procedure: fetch and rebase onto origin/main rather than a possibly stale local main, record each parent's old tip before rebasing it, and move shallowest to deepest so every descendant lands on its newly rebased parent. GitHub retargets a child only after the merged head branch is deleted, and retargeting never rewrites commits -- always rebase, inspect, and rerun CI. Add the repository settings that enforce the convention instead of relying on memory, and keep an empty [Unreleased] after each release. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
# Conflicts: # CHANGELOG.md
sgathrid
added a commit
that referenced
this pull request
Aug 11, 2026
`on.pull_request.branches` filters on a PR's *base*, and a stacked PR targets its parent topic branch. The allowlist `[main, worktree-rust-migration-v0.4.0, 'release/**']` therefore matched no stacked PR at all: #337 (base `chore/rmcp-3x-264`) drew zero Actions runs, so a stacked change was reviewed and merged with no verification behind it. `ci.yml` has no `workflow_dispatch`, so it could not even be triggered by hand. Widening the allowlist to topic-branch globs would admit every PR aimed at anyone's branch, which is not the intent. Instead the trigger is unfiltered and the policy moves into a `gate` job backed by `scripts/ci_gate.py`: * `push` — already filtered by the trigger. Run. * `pull_request` into a `TRUNK_PATTERNS` branch. Run, decided before any API call, so the path to `main` gains no new dependency. * `pull_request` belonging to a GitHub stack, whatever it targets. Run. * Any other `pull_request` — a one-off aimed at a topic branch. Skip. Stack membership is real data, not inferred from branch names: the `PullRequestStack` GraphQL API exposes `pullRequest.stack { number size }`, which returns a stack for #337 and #324 (stack #338, positions 2 and 1) and `null` for non-stacked PRs such as #320 and #318. `--selftest` covers all twelve decision-table cases and runs as the gate's first step, including the `push` path that could otherwise break `main` and a check that `release/*` does not match across a slash. Actions globs are not `fnmatch` — `*` stops at `/` while `**` crosses it — so the matcher is implemented rather than delegated. Unverifiable locally, and deliberately loud about it: whether an Actions installation token can read `PullRequestStack` is a different question from whether the logic is right. The local checks used a personal token. So the gate prints the raw GraphQL response including any `errors` array, never fails its own step, and fails *open* — an unreadable response runs CI with a `::warning::` saying the stacked-PR-only policy is not in effect, rather than silently skipping verification. This PR's own run is the test of that access. No CHANGELOG entry: CI plumbing, per `.agents/AGENTS.md:90`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jeremy-wayland
pushed a commit
that referenced
this pull request
Aug 12, 2026
`on.pull_request.branches` filters on a PR's *base*, and a stacked PR targets its parent topic branch. The allowlist `[main, worktree-rust-migration-v0.4.0, 'release/**']` therefore matched no stacked PR at all: #337 (base `chore/rmcp-3x-264`) drew zero Actions runs, so a stacked change was reviewed and merged with no verification behind it. `ci.yml` has no `workflow_dispatch`, so it could not even be triggered by hand. Widening the allowlist to topic-branch globs would admit every PR aimed at anyone's branch, which is not the intent. Instead the trigger is unfiltered and the policy moves into a `gate` job backed by `scripts/ci_gate.py`: * `push` — already filtered by the trigger. Run. * `pull_request` into a `TRUNK_PATTERNS` branch. Run, decided before any API call, so the path to `main` gains no new dependency. * `pull_request` belonging to a GitHub stack, whatever it targets. Run. * Any other `pull_request` — a one-off aimed at a topic branch. Skip. Stack membership is real data, not inferred from branch names: the `PullRequestStack` GraphQL API exposes `pullRequest.stack { number size }`, which returns a stack for #337 and #324 (stack #338, positions 2 and 1) and `null` for non-stacked PRs such as #320 and #318. `--selftest` covers all twelve decision-table cases and runs as the gate's first step, including the `push` path that could otherwise break `main` and a check that `release/*` does not match across a slash. Actions globs are not `fnmatch` — `*` stops at `/` while `**` crosses it — so the matcher is implemented rather than delegated. Unverifiable locally, and deliberately loud about it: whether an Actions installation token can read `PullRequestStack` is a different question from whether the logic is right. The local checks used a personal token. So the gate prints the raw GraphQL response including any `errors` array, never fails its own step, and fails *open* — an unreadable response runs CI with a `::warning::` saying the stacked-PR-only policy is not in effect, rather than silently skipping verification. This PR's own run is the test of that access. No CHANGELOG entry: CI plumbing, per `.agents/AGENTS.md:90`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jeremy-wayland
pushed a commit
that referenced
this pull request
Aug 12, 2026
`on.pull_request.branches` filters on a PR's *base*, and a stacked PR targets its parent topic branch. The allowlist `[main, worktree-rust-migration-v0.4.0, 'release/**']` therefore matched no stacked PR at all: #337 (base `chore/rmcp-3x-264`) drew zero Actions runs, so a stacked change was reviewed and merged with no verification behind it. `ci.yml` has no `workflow_dispatch`, so it could not even be triggered by hand. Widening the allowlist to topic-branch globs would admit every PR aimed at anyone's branch, which is not the intent. Instead the trigger is unfiltered and the policy moves into a `gate` job backed by `scripts/ci_gate.py`: * `push` — already filtered by the trigger. Run. * `pull_request` into a `TRUNK_PATTERNS` branch. Run, decided before any API call, so the path to `main` gains no new dependency. * `pull_request` belonging to a GitHub stack, whatever it targets. Run. * Any other `pull_request` — a one-off aimed at a topic branch. Skip. Stack membership is real data, not inferred from branch names: the `PullRequestStack` GraphQL API exposes `pullRequest.stack { number size }`, which returns a stack for #337 and #324 (stack #338, positions 2 and 1) and `null` for non-stacked PRs such as #320 and #318. `--selftest` covers all twelve decision-table cases and runs as the gate's first step, including the `push` path that could otherwise break `main` and a check that `release/*` does not match across a slash. Actions globs are not `fnmatch` — `*` stops at `/` while `**` crosses it — so the matcher is implemented rather than delegated. Unverifiable locally, and deliberately loud about it: whether an Actions installation token can read `PullRequestStack` is a different question from whether the logic is right. The local checks used a personal token. So the gate prints the raw GraphQL response including any `errors` array, never fails its own step, and fails *open* — an unreadable response runs CI with a `::warning::` saying the stacked-PR-only policy is not in effect, rather than silently skipping verification. This PR's own run is the test of that access. No CHANGELOG entry: CI plumbing, per `.agents/AGENTS.md:90`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jeremy-wayland
added a commit
that referenced
this pull request
Aug 14, 2026
* ci: run CI on stacked PRs, gated on real stack membership `on.pull_request.branches` filters on a PR's *base*, and a stacked PR targets its parent topic branch. The allowlist `[main, worktree-rust-migration-v0.4.0, 'release/**']` therefore matched no stacked PR at all: #337 (base `chore/rmcp-3x-264`) drew zero Actions runs, so a stacked change was reviewed and merged with no verification behind it. `ci.yml` has no `workflow_dispatch`, so it could not even be triggered by hand. Widening the allowlist to topic-branch globs would admit every PR aimed at anyone's branch, which is not the intent. Instead the trigger is unfiltered and the policy moves into a `gate` job backed by `scripts/ci_gate.py`: * `push` — already filtered by the trigger. Run. * `pull_request` into a `TRUNK_PATTERNS` branch. Run, decided before any API call, so the path to `main` gains no new dependency. * `pull_request` belonging to a GitHub stack, whatever it targets. Run. * Any other `pull_request` — a one-off aimed at a topic branch. Skip. Stack membership is real data, not inferred from branch names: the `PullRequestStack` GraphQL API exposes `pullRequest.stack { number size }`, which returns a stack for #337 and #324 (stack #338, positions 2 and 1) and `null` for non-stacked PRs such as #320 and #318. `--selftest` covers all twelve decision-table cases and runs as the gate's first step, including the `push` path that could otherwise break `main` and a check that `release/*` does not match across a slash. Actions globs are not `fnmatch` — `*` stops at `/` while `**` crosses it — so the matcher is implemented rather than delegated. Unverifiable locally, and deliberately loud about it: whether an Actions installation token can read `PullRequestStack` is a different question from whether the logic is right. The local checks used a personal token. So the gate prints the raw GraphQL response including any `errors` array, never fails its own step, and fails *open* — an unreadable response runs CI with a `::warning::` saying the stacked-PR-only policy is not in effect, rather than silently skipping verification. This PR's own run is the test of that access. No CHANGELOG entry: CI plumbing, per `.agents/AGENTS.md:90`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(ci): correct how TRUNK_PATTERNS relates to the trigger filters The comment described `TRUNK_PATTERNS` as mirroring the old `on.pull_request.branches` allowlist "plus `on.push.branches`", but the two differ: the push filter has no `release/**`. Reading it as a union would suggest a push to `release/*` consults the list, when the trigger never fires for one at all — pushes are filtered before the gate runs, and `decide()` admits any non-PR event outright. Also states plainly that nothing enforces the correspondence with the YAML, so the sync is by hand. Comments only; no behavior change. `--selftest` 12/12, `actionlint` clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * remove rust migration branch --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: jeremy-wayland <jeremy.don.wayland@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
suggestions.rstold agents to extract helper functions whencfg.cyclomaticfailed. Butcfg.cyclomaticis a whole-file sum — every function contributes +1 just by existing. Extracting a helper preserves every decision and adds one more function, so it can only move this number up. The remediation was self-defeating.Fix
Reword the message to name the levers that actually reduce a whole-file sum, and to make the trade-off explicit:
Before
After
The two metrics moving in opposite directions is correct and useful — it is a real trade-off between per-function readability (
ast.max_function_complexity) and whole-file surface area (cfg.cyclomatic). The bug was presenting one side as an imperative with no sign a trade-off existed. Now the reader picks based on which matters for the module.Scope
Guidance wording only. No change to metric computation, gates, or thresholds.
cfg.cyclomaticremains advisory (gates_achieved: false, #193) — this is not a gate-correctness change.Verification
cargo test -p topos-engine— 371 passed.cargo test -p topos-mcp— 105 passed (the context-budget ratchet covers tool definitions, not suggestion text, and is unaffected).cargo fmt --all -- --checkclean.Closes #286
🤖 Generated with Claude Code