Skip to content

docs(guidelines): scope PR ceremony to change size [INBOX-20] - #10030

Draft
saltas888 wants to merge 2 commits into
developfrom
pha/conventions
Draft

docs(guidelines): scope PR ceremony to change size [INBOX-20]#10030
saltas888 wants to merge 2 commits into
developfrom
pha/conventions

Conversation

@saltas888

@saltas888 saltas888 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Why

Harvesting review lessons from PR #10018 (INBOX-20) via /harvesting-review.

Reviewer ajtmccarty asked for that PR to be trimmed to just the 5-line
repository.py annotation fix:

The author agreed and trimmed both in commit f86e14b5. Neither constraint
was written down anywhere, so the speckit pipeline that generated the
original PR had no way to know to right-size itself for a trivial,
no-behavior-change fix — the next trivial card would hit the same review
round-trip.

Investigation (full trail, so the fix doesn't over-generalize): repo
history shows dev/specs/ scaffolding and changelog fragments are the
norm and get merged for real features (repository-organization.md
already documents specs/ archiving/graduating). So the durable rule is
not "never commit these" — that would contradict established practice
and everything the speckit-opsmill-* pipeline exists to produce. The
narrow, correct rule is proportionality: skip/trim the ceremony
specifically when the actual change is trivial and self-explanatory
(e.g. an annotation-only fix with zero behavior change).

What changed

Docs only, no behavioral change:

  • dev/guidelines/changelog.md — new "When to Skip" section: no fragment
    for changes with no user-facing effect; housekeeping is not a
    catch-all for anything code-adjacent.
  • dev/guidelines/repository-organization.md — new "Proportionality"
    note under the specs/ lifecycle: the design record should scale with
    the change, not be a fixed-cost byproduct of running the workflow.
  • dev/guidelines/git-workflow.md — cross-reference both from the Pull
    Requests section (already linked from root AGENTS.md's Coding
    Standards, so the rule is reachable without hunting for it).

Not a lesson (checked, set aside): the cubic-dev-ai[bot] review was a
clean automated pass-through with nothing actionable; the two empty
saltas888 review entries and the CodSpeed performance comment carry no
content.

How to test

Docs-only; no code paths affected. uv run invoke docs.lint doesn't cover
dev/guidelines/** (it globs docs/docs/** only) — verified by reading
tasks/docs.py.

🤖 Generated with Claude Code


Also: harvest from PR #10079 (INBOX-8)

Same branch, second harvesting-review pass — this time over
opsmill/infrahub#10079,
which hit three review threads plus an approving-review aside. Judged each
for whether it generalizes beyond that one PR:

New lesson addeddev/guidelines/backend/python.md, new Type Hints
subsection "str satisfies Sequence — exclude it before narrowing".
Reviewer reaction to the carve-out in #10079
(isinstance(data, str) or not isinstance(data, Sequence)) was
"definitely would have missed this one"
a near-miss worth writing down, not just a correction. str satisfying
Sequence is a classic Python footgun; nothing in the guidelines named it,
and it recurs anywhere a param is widened from an invariant list[T] to a
covariant Sequence[T] whose T includes str.

Already covered, no edit — the other two inline threads
("don't think this requires a changelog"
on an internal typing change, and
"not sure if we need all this spec stuff for ~10 lines of production code change"
re: dev/specs/) restate exactly the two rules this same PR already added
above for INBOX-20. Confirmed by branch analysis: commit c1b89c433 (this
PR) is only on pha/conventions, absent from origin/develop, and
PR #10079's branch (pha/INBOX-8) forked from develop — so the agent
working that card structurally could not have seen guidance that hasn't
merged yet. Not a documentation gap; the fix ships once this PR merges.
saltas888's reply on that thread had already flagged this exact
pipeline-policy question up to this card rather than deciding it inline.

Discarded — the approving review's aside that PeerWithRelationshipMetadata
is overused as both the read model (populated from query results in
manager.py) and the write model (sparse, update-intent-only in
node/create.py) is accurate (confirmed by reading both call sites) but
scoped to that one class, not a recurring authoring convention — no
guideline would have prevented it and none would prevent its recurrence
elsewhere. Left as a standalone architectural observation for separate
tech-debt triage rather than encoded here.

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com


Summary by cubic

Clarifies that PR ceremony should match change size: for trivial, no‑behavior‑change fixes (e.g., INBOX‑20), submit only the code diff and skip changelog fragments and dev/specs/ scaffolding. Updates dev/guidelines/changelog.md, repository-organization.md, and git-workflow.md with this guidance and cross‑references.

Written for commit c1b89c4. Summary will update on new commits.

Review in cubic

PR #10018 (INBOX-20) got a reviewer ask for the whole PR to be just the
5-line repository.py annotation fix: no dev/specs/ spec-kit scaffolding,
no changelog fragment for a change with no user-facing effect. Neither
constraint was written down anywhere, so an agent following the normal
speckit pipeline had no way to know to trim them. Document both:

- changelog.md: skip the fragment when a change has no user-facing
  effect; housekeeping is not a catch-all for anything code-adjacent.
- repository-organization.md: the spec-kit design record should be
  proportional to the change it documents, not a fixed-cost byproduct
  of running the workflow.
- git-workflow.md: cross-reference both from the Pull Requests section
  so the rule is reachable from root AGENTS.md's Coding Standards link.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013MwudkACUQdCZTJ5aVBnr3

@cubic-dev-ai cubic-dev-ai Bot 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.

2 issues found across 3 files

Confidence score: 5/5

  • In dev/guidelines/changelog.md, targeting develop for a docs-only change conflicts with the base-branch policy, which can cause process drift and delayed guidance updates if merged as-is — retarget this PR to stable to align with the release flow.
  • In dev/guidelines/repository-organization.md, the 'Git Workflow → Pull Requests' cross-reference points to the file root instead of the section, so readers may miss the intended instructions and navigation becomes inconsistent — update the link to include #pull-requests.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="dev/guidelines/changelog.md">

<violation number="1" location="dev/guidelines/changelog.md:55">
P3: This docs-only change targets develop, but per the base-branch rules, changes that can't affect a running product (including docs, tooling, CI) should target stable. Consider retargeting to stable so the guidance ships sooner rather than waiting for the next minor release train.</violation>
</file>

<file name="dev/guidelines/repository-organization.md">

<violation number="1" location="dev/guidelines/repository-organization.md:89">
P3: The cross-reference link says 'Git Workflow → Pull Requests' but points only to the file root, not the Pull Requests section. Append `#pull-requests` to the link so the text and target match, consistent with how the git-workflow.md change correctly uses `changelog.md#when-to-skip` with a section anchor.</violation>
</file>

Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic


## When to Skip

Skip the fragment entirely when the change has **no user-facing effect** — an internal

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.

P3: This docs-only change targets develop, but per the base-branch rules, changes that can't affect a running product (including docs, tooling, CI) should target stable. Consider retargeting to stable so the guidance ships sooner rather than waiting for the next minor release train.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At dev/guidelines/changelog.md, line 55:

<comment>This docs-only change targets develop, but per the base-branch rules, changes that can't affect a running product (including docs, tooling, CI) should target stable. Consider retargeting to stable so the guidance ships sooner rather than waiting for the next minor release train.</comment>

<file context>
@@ -50,6 +50,14 @@ uv run towncrier create -c "Added breadcrumb navigation for hierarchical schemas
 
+## When to Skip
+
+Skip the fragment entirely when the change has **no user-facing effect** — an internal
+type-annotation correction, a refactor with no behavior change, cleanup of internal docs or
+spec-kit scaffolding. `housekeeping` is for internal changes a user could still plausibly notice
</file context>

- Moved to `guidelines/` (if describing how to use something)

**Proportionality**: Not every change needs a spec. If the actual code change is small enough to
be self-explanatory (e.g. an annotation-only fix, a one-line correction with no behavior change),

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.

P3: The cross-reference link says 'Git Workflow → Pull Requests' but points only to the file root, not the Pull Requests section. Append #pull-requests to the link so the text and target match, consistent with how the git-workflow.md change correctly uses changelog.md#when-to-skip with a section anchor.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At dev/guidelines/repository-organization.md, line 89:

<comment>The cross-reference link says 'Git Workflow → Pull Requests' but points only to the file root, not the Pull Requests section. Append `#pull-requests` to the link so the text and target match, consistent with how the git-workflow.md change correctly uses `changelog.md#when-to-skip` with a section anchor.</comment>

<file context>
@@ -85,6 +85,12 @@ Each directory in `dev/` serves a specific purpose and follows a content lifecyc
 - Moved to `guidelines/` (if describing how to use something)
 
+**Proportionality**: Not every change needs a spec. If the actual code change is small enough to
+be self-explanatory (e.g. an annotation-only fix, a one-line correction with no behavior change),
+skip the spec-kit scaffolding entirely or trim `dev/specs/<feature>/` from the PR before it goes
+to review — the design record should be proportional to the change it documents, not a fixed-cost
</file context>

PR #10079 widened RelationshipManager.update()'s data parameter from an
invariant list to a covariant Sequence to drop a call-site type: ignore.
The reviewer's "definitely would have missed this one" was for the
carve-out that makes it safe: str satisfies Sequence, so isinstance(data,
str) must be checked before isinstance(data, Sequence), or a bare peer id
gets iterated character-by-character. Nothing in the guidelines named this
gotcha, so document it next to the neighboring isinstance-narrowing and
list/Sequence-variance sections.

The PR's other two review threads (skip-changelog for no-user-visible-
effect changes, and proportional dev/specs/ ceremony) restate lessons
already codified on this branch by c1b89c4 (INBOX-20) — that commit
just hasn't reached develop yet, so PR #10079's branch never saw it. No
doc gap, no edit needed there.

The approving review's PeerWithRelationshipMetadata read/write-split
suggestion is a real, confirmed observation (manager.py and node/create.py
populate it asymmetrically) but is scoped to that one class, not a
recurring authoring convention — discarded from this harvest as PR-local
tech debt rather than encoded as a rule.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WgpvpBsruPYD3fYpyfzL7R

@cubic-dev-ai cubic-dev-ai Bot 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.

0 issues found across 1 file (changes from recent commits).

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Shadow auto-approve: would not auto-approve. Auto-approval blocked by 2 unresolved issues from previous reviews.

Re-trigger cubic

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.

1 participant