Skip to content

Bring the publisher handler back under the complexity law, and report why the law had no teeth #256

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Antawari merged 5 commits into from
Jul 28, 2026
Merged

Bring the publisher handler back under the complexity law, and report why the law had no teeth #256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fd5b331
Bring the publisher handler's handle back under the complexity law
Antawari Jul 28, 2026
bcdd11a
Anchor the publisher handler's suppression to its symbol, not to a line
Antawari Jul 28, 2026
46570c7
Raise the handlers package ceiling by the 30 lines the refactor costs
Antawari Jul 28, 2026
f32a599
Merge remote-tracking branch 'origin/main' into catrina/enforce-the-c…
Antawari Jul 28, 2026
18f7545
Merge remote-tracking branch 'origin/main' into catrina/enforce-the-c…
Antawari Jul 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion exemptions.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@
},
{
"file": "src/bonfire/handlers/bard.py",
"symbol_or_line": "282",
"symbol_or_line": "BardHandler.handle",
"rule": "BLE001",
"reason": "Handler boundary that must salvage partial PR-flow progress (branch/sha/staged files) on ANY mid-flow failure; the partial-metadata capture depends on catching all.",
"approver": "BubbleGum/Elegance-Law architectural blessing (BON-1757); Anta gates via PR merge"
Expand Down
10 changes: 9 additions & 1 deletion file-budget.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@
"packages": {
"src/bonfire/dispatch": 2869,
"src/bonfire/engine": 1981,
"src/bonfire/handlers": 3250,
"src/bonfire/handlers": 3280,
"src/bonfire/onboard": 4460,
"tests/integration": 1702,
"tests/unit": 73777
Expand Down Expand Up @@ -174,6 +174,14 @@
"reason": "tests/unit measured EXACTLY 73199 on bare origin/main -- the ceiling and the measurement were the same number, so the package had zero headroom and no pull request could add a unit test at all. What the 211 lines buy: tests/unit/test_init_first_run_refusals.py, eight tests over the first command a stranger runs. bonfire init met four hostile-but-ordinary path shapes with a raw Python traceback or, in one case, with a success claim -- a directory named bonfire.toml satisfied Path.exists(), the write was skipped, and the success block printed 'Already present: bonfire.toml (project config)' with exit code 0 over a project no Bonfire command can read. Seven of the eight tests fail on origin/main's behaviour with the fix removed, each for its own stated reason (PermissionError from the first write, PermissionError from the gitignore append, exit 0 with the success banner, IsADirectoryError from inside the safe-read helper, and FileExistsError three times out of mkdir(exist_ok=True) for a regular file, a second regular file, and a dangling symlink). The eighth passes on both sides on purpose: it is the control rod against a guard that learns to refuse everything, and without it the other seven would be satisfied by a command that refuses unconditionally. Four distinct defects, seven distinct failure modes, one negative control -- that is the smallest honest count, not a padded one. Explicitly rejected, in order of how tempting each was: (1) shipping fewer tests to fit -- the control rods ARE the deliverable, and four defects verified by two tests is a weaker claim than the one the pull request makes; (2) parking the file in an unbudgeted tree (tests/smoke, tests/dispatch), which costs zero budget and is dodging the ratchet rather than restructuring, and buries a regression contract where nobody maintaining init would look for it; (3) tests/integration, which is frozen at 1363 with zero headroom of its own and is the wrong home anyway for single-command CLI tests; (4) shrinking another test file to make room, which would have raced two other lanes live in this tree for budget and is worse than asking; (5) a purpose entry for the new file, which is an exemption in everything but name and would drop the file out of the ratchet permanently to accommodate one change -- the same trade the dispatch and handlers raises below both rejected, and exemptions.json is at 52/52 besides. NOTE for the next reader: a ratchet that reaches its own measurement has stopped preventing bloat and started taxing test coverage. Every future unit test in this repo now needs a raise. That is a design question about the tests/unit ceiling, not something a lane can settle.",
"approver": "Anta gates via PR merge -- raise surfaced in the PR body and in the lane report, not taken silently"
},
{
"package": "src/bonfire/handlers",
"from": 3250,
"to": 3280,
"lines": 30,
"reason": "src/bonfire/handlers measured EXACTLY 3250 on bare origin/main -- the ceiling and the measurement were the same number, so the package had zero headroom and no behaviour-preserving complexity refactor could land in it at all. BardHandler.handle measured 19 against a limit of 10 and was never in complexipy-snapshot.json: drift, not grandfathered debt, and it landed because CI does not run the complexity gate (the pinned quality workflow lists its steps by hand and complexipy is not among them). What the 30 lines buy: three extractions that take handle from 19 to 7, each measured so the branches are removed rather than relocated -- _create_branch 6 (the branch-creation try/except and its 'already exists' probe), _partial_metadata 3 (the salvage map on the throw path), _failed 0 (the FAILED envelope four refusal paths each built by hand). The suite is the control and its counts are identical on both sides: 5746 passed, 3 skipped, 37 xfailed, 20 xpassed. The lines are what an extraction costs and nothing else -- a signature, a docstring, and a call site, three times over. Explicitly rejected, in order of how tempting each was: (1) adding the function to complexipy-snapshot.json, which is the exact move the remediation programme exists to end and would have cost zero budget; (2) the seven-line minimum variant, which reaches handle 9 by compressing the salvage map into a table-and-filter -- it was measured for complexity and for lines and never run against the suite, and an unproven refactor is not cheaper, it is unmeasured; (3) deleting comments or blank lines to buy the lines back, which is gaming, and pointless besides because the gauge measures max(physical, statements) so compression cannot manufacture headroom; (4) splitting the helpers into a new file carrying a purpose entry, which is the tool's own designed escape hatch and would have made the package total FALL rather than rise -- rejected because it drops a live file out of the ratchet permanently to accommodate one change, the same trade the three raises above all rejected; (5) moving the helpers outside src/bonfire/handlers into an unbudgeted package, which costs zero budget and is dodging the ratchet, and puts a publisher-specific envelope builder somewhere no one maintaining the publisher would look; (6) hunting dead code in the package to fund the lines -- five candidates checked, every one live. NOTE for the next reader, and it is the finding that explains why this debt never moves: every handler file holding an un-grandfathered violation also carries a suppression that exemptions.json pinned by LINE NUMBER (bard 282, wizard 492, steward 166, sage_correction_bounce 320/488/539, preflight_pytest 321, architect 72/214). Any refactor of any of those functions shifts a pinned line and turns cf-exemptions red, so the register that records accepted debt structurally forbids the refactoring that would retire it. This lane re-anchored only its own row, to a symbol; the other eight are still line-pinned. wizard.py was left untouched on purpose: at 499 against the 500-line new-file cap, in a package that had zero headroom, with its own suppression pinned at line 492, it is blocked three ways before the first edit.",
"approver": "Anta gates via PR merge -- raise surfaced in the PR body and in the lane report, not taken silently"
},
{
"package": "src/bonfire/engine/pipeline.py",
"from": 989,
Expand Down
212 changes: 121 additions & 91 deletions src/bonfire/handlers/bard.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,51 @@ def _slugify_task(task: str, envelope_id: str) -> str:
return f"{truncated}-{envelope_id[:_SLUG_ID_LEN]}"


def _failed(
envelope: Envelope,
stage: StageSpec,
*,
metadata: dict[str, Any],
error_type: str,
message: str,
) -> Envelope:
"""Build the FAILED envelope every refusal path in this handler returns.

*metadata* is layered over ``envelope.metadata``, which is what each
inlined ``model_copy`` did by hand.
"""
return envelope.model_copy(
update={
"metadata": {**envelope.metadata, **metadata},
"error": ErrorDetail(
error_type=error_type,
message=message,
stage_name=stage.name,
),
"status": TaskStatus.FAILED,
},
)


def _partial_metadata(
*,
branch_name: str | None,
base_sha: str | None,
commit_sha: str | None,
staged_paths: list[str],
) -> dict[str, Any]:
"""Carry forward whatever the run got far enough to learn before it threw."""
partial: dict[str, Any] = {}
if branch_name is not None:
partial[_META_BRANCH] = branch_name
if base_sha is not None:
partial[_META_BASE_SHA] = base_sha
if commit_sha is not None:
partial[_META_COMMIT_SHA] = commit_sha
partial[_META_STAGED_FILES] = json.dumps(staged_paths)
return partial


# ---------------------------------------------------------------------------
# Handler
# ---------------------------------------------------------------------------
Expand Down Expand Up @@ -138,6 +183,28 @@ def __init__(
# templating) can read it without a constructor churn.
self._config = config

async def _create_branch(self, branch_name: str) -> str | None:
"""Create *branch_name*; ``None`` reports an "already exists" collision.

Takes the created name back rather than assuming it: ``create_branch``
owns the ``bonfire/`` prefix, and push/PR head/metadata must all name
the ref that actually exists. Every other ``RuntimeError`` propagates.
"""
try:
created = await self._git_workflow.create_branch(branch_name)
if not isinstance(created, str) or not created:
# Falling back to the requested name IS the original
# defect: it lacks the prefix, so the ref would not exist.
raise RuntimeError(
"create_branch did not return the branch it created "
f"(got {created!r}); cannot determine the ref to push."
)
except RuntimeError as branch_exc:
if "already exists" in str(branch_exc):
return None
raise
return created

async def handle(
self,
stage: StageSpec,
Expand All @@ -158,23 +225,16 @@ async def handle(

# 2. Empty-artifacts precondition: fail BEFORE any git call.
if not staged_paths:
return envelope.model_copy(
update={
"metadata": {
**envelope.metadata,
_META_STAGING_FAILURE_REASON: "empty_artifacts",
},
"error": ErrorDetail(
error_type="empty_artifacts",
message=(
"BardHandler refused to commit: envelope.artifacts "
"contains no file_written/file_modified entries. "
f"envelope_id={envelope.envelope_id}, stage={stage.name}."
),
stage_name=stage.name,
),
"status": TaskStatus.FAILED,
},
return _failed(
envelope,
stage,
metadata={_META_STAGING_FAILURE_REASON: "empty_artifacts"},
error_type="empty_artifacts",
message=(
"BardHandler refused to commit: envelope.artifacts "
"contains no file_written/file_modified entries. "
f"envelope_id={envelope.envelope_id}, stage={stage.name}."
),
)

# 3. Capture base SHA at entry -- before any branch-moving operation.
Expand All @@ -183,41 +243,23 @@ async def handle(
# 4. Build branch name (no leading "bonfire/" -- create_branch adds it).
branch_name = f"{_BRANCH_KIND}/{_slugify_task(envelope.task, envelope.envelope_id)}"

# 5. Create branch; structured error on collision. Take the name
# back rather than assuming: create_branch owns the prefix, and
# push/PR head/metadata must all name the ref that exists.
try:
created = await self._git_workflow.create_branch(branch_name)
if not isinstance(created, str) or not created:
# Falling back to the requested name IS the original
# defect: it lacks the prefix, so the ref would not exist.
raise RuntimeError(
"create_branch did not return the branch it created "
f"(got {created!r}); cannot determine the ref to push."
)
branch_name = created
except RuntimeError as branch_exc:
if "already exists" in str(branch_exc):
return envelope.model_copy(
update={
"metadata": {
**envelope.metadata,
_META_BRANCH: branch_name,
_META_BASE_SHA: base_sha,
_META_STAGING_FAILURE_REASON: "branch_collision",
},
"error": ErrorDetail(
error_type="branch_collision",
message=(
f"Branch {branch_name!r} already exists; "
"refusing to rewrite history."
),
stage_name=stage.name,
),
"status": TaskStatus.FAILED,
},
)
raise
# 5. Create branch; structured error on collision.
created = await self._create_branch(branch_name)
if created is None:
return _failed(
envelope,
stage,
metadata={
_META_BRANCH: branch_name,
_META_BASE_SHA: base_sha,
_META_STAGING_FAILURE_REASON: "branch_collision",
},
error_type="branch_collision",
message=(
f"Branch {branch_name!r} already exists; refusing to rewrite history."
),
)
branch_name = created

# 6. Stage + commit. Returns full HEAD SHA.
commit_sha = await self._git_workflow.commit(
Expand All @@ -227,28 +269,23 @@ async def handle(

# 7. Post-commit assert: did the commit actually introduce a diff?
if commit_sha == base_sha:
return envelope.model_copy(
update={
"metadata": {
**envelope.metadata,
_META_BRANCH: branch_name,
_META_BASE_SHA: base_sha,
_META_COMMIT_SHA: commit_sha,
_META_STAGED_FILES: json.dumps(staged_paths),
_META_STAGING_FAILURE_REASON: "no_diff_after_commit",
},
"error": ErrorDetail(
error_type="no_diff_after_commit",
message=(
f"BardHandler detected phantom commit: HEAD SHA "
f"{commit_sha} equals base ({self._base_branch}) "
f"SHA {base_sha}. No changes were introduced -- "
"refusing to push or open PR."
),
stage_name=stage.name,
),
"status": TaskStatus.FAILED,
return _failed(
envelope,
stage,
metadata={
_META_BRANCH: branch_name,
_META_BASE_SHA: base_sha,
_META_COMMIT_SHA: commit_sha,
_META_STAGED_FILES: json.dumps(staged_paths),
_META_STAGING_FAILURE_REASON: "no_diff_after_commit",
},
error_type="no_diff_after_commit",
message=(
f"BardHandler detected phantom commit: HEAD SHA "
f"{commit_sha} equals base ({self._base_branch}) "
f"SHA {base_sha}. No changes were introduced -- "
"refusing to push or open PR."
),
)

# 8. Push (keyword-only branch arg -- GitWorkflow.push signature).
Expand Down Expand Up @@ -280,22 +317,15 @@ async def handle(
)

except Exception as exc: # noqa: BLE001
partial_metadata: dict[str, Any] = {**envelope.metadata}
if branch_name is not None:
partial_metadata[_META_BRANCH] = branch_name
if base_sha is not None:
partial_metadata[_META_BASE_SHA] = base_sha
if commit_sha is not None:
partial_metadata[_META_COMMIT_SHA] = commit_sha
partial_metadata[_META_STAGED_FILES] = json.dumps(staged_paths)
return envelope.model_copy(
update={
"metadata": partial_metadata,
"error": ErrorDetail(
error_type=type(exc).__name__,
message=str(exc),
stage_name=stage.name,
),
"status": TaskStatus.FAILED,
},
return _failed(
envelope,
stage,
metadata=_partial_metadata(
branch_name=branch_name,
base_sha=base_sha,
commit_sha=commit_sha,
staged_paths=staged_paths,
),
error_type=type(exc).__name__,
message=str(exc),
)
Loading