Repository navigation
✨ House-style section every kick opens with: repo HIVE_STYLE.md > hive default > built-in - #14582
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Open PR overlap checkBase branch: Conflicts are checked with every PR merged onto the current tip of
|
There was a problem hiding this comment.
Read at ad63b34 against v5 merge-base 7d61076. The resolver and kick wiring look correct to me; two low-severity notes and the rebase below.
Verified
pkg/housestyle.Resolve(housestyle.go:114-150): repo → hive → built-in, each candidate used whole or skipped with a recordedSkip; missing files are silent, unreadable ones audited (:155-170).Capfloors at 512 and defaults to 4096 as documented.scheduler.resolveHouseStyle(kick_housestyle.go:30-62) scans only the repo candidate, viaioscan.ScanInputFrom(…, SurfaceIssueBody, policy), and reusesrecordIoscanBlock/ioscanAuditUser/auditLoggerfromioscan_enforce.go; the repo root comes from the sameagentsRepoRoottheAGENTS.mdprimer uses (kick_priming.go:36).addHouseStyleis first instaticSeams(kick_layout.go:83), so after the reverse application it is the first section under the kick header, as the PR says.- Dashboard:
StyleSourceis derived fromproc.LastKickMessage(status_builder.go:1166) viaSourceFromKick, which only accepts the three known values — reports what was actually sent, as claimed.
Low — inline oversize skips style_file instead of falling through to it. housestyle.go:131-141: when project.style is non-empty but over the cap, the else if in.HiveFile != "" branch is never reached, so the result is built-in even when style_file is set and valid. docs/house-style.md:28-29 says "when one is skipped the next applies"; an operator with both set would reasonably expect the file. TestResolveSizeCap (housestyle_test.go:66-68) pins the built-in outcome, but with no HiveFile, so it does not cover this case. Cheap fix: drop the else and let the file branch run when inline was skipped.
Low — settings parity satisfied by tooltip text only. project.style and project.style_file pass TestSettingsDialogReferencesEveryHiveYAMLLeaf because the strings appear in a tooltip (static/index.html:13526), not because the Settings dialog has a control for them, while project.style_max_bytes is allowlisted with # TODO #14575 (settings_parity_allowlist.txt:345). Either all three belong on the allowlist under the same TODO, or all three get a control; as is, the gate reports two of them as "referenced" that an operator cannot edit from the UI.
Rebase: four files in this diff also changed on v5 since the merge-base and GitHub reports the PR as conflicting: src/docs/api-reference.md, src/pkg/dashboard/server.go, src/pkg/dashboard/static/index.html, src/pkg/dashboard/status_builder.go. CI (test, coverage, build) did not run on this head for that reason; the pkg/dashboard tests in the body are therefore unconfirmed by CI.
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Read pkg/housestyle, the scheduler seam and the dashboard readback against v5 at merge-base 7d61076. The logic looks correct to me; the only thing between this and green is the rebase.
What I verified:
- Precedence and fall-through (
housestyle.go:108-144): a repo file that is oversize, unreadable, blank or scanner-blocked records aSkipand falls to hive inline → hive file → built-in; nothing is ever clipped, andResolvecannot return empty text.Cap(:96-104) applies the 4096 default / 512 floor the docs state. - Ordering:
addHouseStyleis first instaticSeams(kick_layout.go:83); since seams insert after the header and are applied in reverse, it ends up first in the static prefix, exactly asTestKickLayoutStaticPrefixIsCacheableis updated to assert. - Dashboard readback is real, not cosmetic:
SourceFromKickparsesproc.LastKickMessage, and that field is assigned the fully assembled kick (crux/agent/manager_kick.go:429viarecordDeliveredKickLocked), sostyleSourcereports what was actually delivered. - Trust posture: the repo file is the only untrusted candidate and it is the only one scanned (
kick_housestyle.go:37-47, issue-body surface, blocked → audit line + skip). For comparison, the existing AGENTS.md priming inkick_priming.goapplies no ioscan at all, so this is stricter than the neighbouring repo-sourced seam, not looser. Per the PR body the "leads every kick" placement is deliberate. - Compiles: CI skipped
buildbecause of the conflict, so I built./pkg/housestyle ./pkg/scheduler ./pkg/dashboard ./pkg/configatad63b34locally — clean.
Rebase note: the four conflicting files are pkg/dashboard/server.go, status_builder.go, static/index.html and docs/api-reference.md. In server.go the collision is the gofmt realignment of the AgentStatus struct that adding StyleSource triggered — every neighbouring field line changed whitespace — so expect a mechanical, not semantic, conflict there. After the rebase, api-reference.md line citations may need bash src/scripts/check-api-reference-citations.sh --fix again.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Read pkg/housestyle, scheduler/kick_housestyle.go, the kick_layout.go hook and the existing AGENTS.md priming path at the PR head. The logic looks correct to me; the branch is conflicting with v5 and needs the rebase the label already asks for.
What I verified:
housestyle.Resolve(housestyle.go:108-146) never returns empty: a blocked, oversize or unreadable repo file falls through to hive inline → hive file →Builtin, and each loser is recorded inSkipped, whichresolveHouseStyleturns into onehouse_style_skipaudit line each (kick_housestyle.go:48-54). Missing files are silent (readCandidate,housestyle.go:152-156), so the common no-file case produces no audit noise.- The repo file is located via the same
agentsRepoRoothelperAGENTS.mduses (kick_priming.go:36-48), so it reads from the primary repo's checkout only. The scan runs on repo text only, with theSurfaceIssueBodypolicy (kick_housestyle.go:38-46); hive-supplied text is trusted. When ioscan is off the repo file applies unscanned — that is the same postureprimeAgentsMdalready has (kick_priming.go:81-112, no scan), so this does not widen the existing trust boundary. insertAfterKickHeader(kick_layout.go:142-151) places the section right after the first header line, ahead of the other static seams, which is what keeps it inside the cacheable prefix.- Precedence repo > hive > built-in is the one #14575 specifies, so I am not raising it as a question.
One note: the PR says Fixes #14575, but #14575's proposal includes a Contents-API fallback for HIVE_STYLE.md when no checkout is wired, which this PR deliberately leaves out (docs/house-style.md:90, "Non-goals" in the body). If that part is meant to stay open as a follow-up, Refs #14575 (with the leftover named) keeps the issue from auto-closing on merge; if you consider the issue done without it, Fixes is right — your call as its author.
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
…VE_STYLE.md > hive default > built-in) Every kick on every backend now begins with a HOUSE STYLE section: how changes are to be made, as distinct from what to work on. A managed repo's root HIVE_STYLE.md (read from project.checkouts_dir, like AGENTS.md) overrides project.style / project.style_file, which override the built-in text. Candidates are capped (project.style_max_bytes, 4096) and skipped whole when oversize; the repo file is untrusted and is refused whole on an ioscan block at the issue-body policy, with a house_style_skip audit line. The agent card and detail pane show which source applied, read back from the kick header (/api/status agents[].styleSource). Fixes #14575 Signed-off-by: Andrew Anderson <andy@clubanderson.com>
… control exists Signed-off-by: Andrew Anderson <andy@clubanderson.com>
Signed-off-by: Andrew Anderson <andy@clubanderson.com>
Signed-off-by: Andrew Anderson <andy@clubanderson.com>
d6cb825 to
02192d6
Compare
Generated by changelog-fragment-guard from the PR title (#5675). Edit the wording freely, or replace this file with the no-changelog label if the change is not user-visible. Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Signed-off-by: clubanderson <407614+clubanderson@users.noreply.github.com>
|
@clubanderson is auto-promoted for contributor-lane merging on this PR: the last 5 merged PRs by this author in this hive's repositories have no revert or sentinel-alert evidence. CI, reviewer verdict, diff fences, soak, holds and branch protection still apply. |
|
@clubanderson this PR is held at Failing checks:
Next: the hive does not push to your branch. Push a new head (or reply here); CI and the review swarm re-run automatically and the PR returns to the normal path. |
There was a problem hiding this comment.
The design and the code read correctly to me; the red CI is three ratchets the diff trips, all mechanical. Nothing here needs a redesign.
What I verified in the tree (head ef7bbd0, base v5)
src/pkg/housestyle/housestyle.go— precedence repo > inline > file > built-in is implemented as described; oversize candidates are skipped whole viareadCandidate, blocked repo text falls through, andResolvecan never return empty (Builtinfloor).SourceFromKickvalidates the parsed source against the three constants, so a kick that merely contains the header text can't inject an arbitrary value into/api/status.src/pkg/scheduler/kick_housestyle.go— only repo-sourced text goes throughin.Scan;ioscan.SurfaceIssueBodyandrecordIoscanBlockare the existing seams. The helpers it leans on all exist on v5 (insertAfterKickHeaderkick_layout.go:141,agentsRepoRootkick_priming.go:36,PrimaryRepoForAgentconfig/agent_repos.go:158,ioscanEnabled/auditLoggerioscan_enforce.go:123,135).kick_layout.go:83— the house-style seam is first instaticSeams, which (applied in reverse) puts it directly under the header. Static per agent, so the cacheable-prefix claim holds.
Why CI is red — each is caused by this diff
changelog-fragment-guard:changelog.d/added-house-style.mdis a prose paragraph; the guard requires every fragment to start with a-bullet ("it IS the entry; the compiler owns the ### headings").changelog.d/added-14582.mdalready carries a valid one-line entry — dropadded-house-style.md(or fold its text into the bullet).test (rest 3/4)—TestDashboardLoginNeverRequestsRepoScope:pkg/github/deviceflow_test.go:213forbids the regex"repo"anywhere inpkg/dashboard/server.go. The new field comment atserver.go:778contains the literal"repo"(in quotes). It is a comment, not a scope request, so this is a lexical false positive against the guard — but it will keep failing until the quotes come off (e.g.`repo`orrepo).test (rest 2/4)—TestDashboardImportRatchet:status_builder.go:27addspkg/dashboard → pkg/housestyle, which is not insrc/internal/testutil/dashboard_import_allowlist.txt(the PR does not touch that file). Either add it with a one-line justification, or moveSourceFromKickbehind an existing seam the dashboard already imports.
coverage and the aggregate test job fail only because the rest shard failed; they should clear with 2 and 3.
One mismatch between body and code, minor: the body says the hive default is "project.style inline, else project.style_file" — that is what Resolve does, but note a blank-after-trim project.style falls through to style_file while a non-blank oversize project.style records a Skip and does not try style_file (housestyle.go Resolve, the inline branch returns before the else if). Probably intended — an operator who set inline text meant inline — but worth a sentence in docs/house-style.md since the two cases behave differently.
Correctness of the feature itself looks fine to me; this is a three-line fix-up for the author.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Read pkg/housestyle, scheduler/kick_housestyle.go, the kick_layout.go seam, and the dashboard wiring against v5. The feature logic looks correct to me (precedence, cap, ioscan refusal → fall-through, static-prefix placement). The red CI is three mechanical things, all in this diff:
1. TestDashboardImportRatchet — test (rest 2/4) (medium)
src/pkg/dashboard/status_builder.go:27 adds an import of pkg/housestyle, which is not in src/internal/testutil/dashboard_import_allowlist.txt. The ratchet fails with "pkg/dashboard gained a new dependency on pkg/housestyle … add it to the allowlist with justification in the PR". Either add the line, or have the scheduler stamp the source onto the process record so the dashboard doesn't need the package just to call SourceFromKick.
2. TestDashboardLoginNeverRequestsRepoScope — test (rest 3/4) (medium)
The new comment at src/pkg/dashboard/server.go:778 contains the literal "repo" (with quotes). src/pkg/github/deviceflow_test.go:216 greps server.go for "repo" as a #1927 scope guard and does not distinguish comments from code. Drop the quotes in the comment (repo / hive / built-in) and the test passes; nothing about the login path actually changed.
3. changelog.d/added-house-style.md is malformed and redundant (medium)
src/scripts/compile-changelog.sh:119 requires a fragment's first line to start with - (or a <!-- release: marker). This file's first line is prose ("Every kick now opens with…"), so the next tagged v5 release will abort on it — the same class of failure #14593 is currently fixing for docs-crux-stage4.md. changelog.d/added-14582.md already carries the one-line entry; I'd delete added-house-style.md outright.
One low note, not a blocker: housestyle.go:148 (} else if in.HiveFile != "") means an oversize project.style falls straight to built-in rather than trying style_file, while the StyleMaxBytes doc at config.go:969 says "the next source applies". Either behaviour is fine; the comment and code should just agree.
The coverage failure is downstream of the rest shards and should clear with the above.
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
Signed-off-by: Andrew Anderson <andy@clubanderson.com>
… scope-guard literal in comment pkg/dashboard → pkg/housestyle is a one-way read of the style source already recorded on the kick message (no new seam exists for kick metadata); the login-scope guard matches the quoted word in a struct comment, so reword. Signed-off-by: Andrew Anderson <andy@clubanderson.com>
…ate) Signed-off-by: Andrew Anderson <andy@clubanderson.com>
There was a problem hiding this comment.
Read at 6137ba4 against the tree. The precedence logic and the untrusted-repo handling do what the body says; no defects found.
Verified
housestyle.Resolve(src/pkg/housestyle/housestyle.go:108–141): repo file → hive inline → hive file → built-in, each candidate skipped whole on oversize or scanner block with aSkiprecorded, never truncated. Oversize inlineproject.stylefalls to built-in without tryingstyle_file— consistent with the documented "inline, else file".- Scanner runs on repo text only (
housestyle.go:116;scheduler/kick_housestyle.go:40usesSurfaceIssueBody), and the skip path writes thehouse_style_skipaudit line per candidate as claimed. insertAfterKickHeader(kick_layout.go:142–151) puts the section directly under the first line, and the layout wiring atkick_layout.go:83keeps it inside the static prefix.SourceFromKickonly accepts the three known source names, so a repo file containing the header string cannot spoof the dashboard field (the real header always precedes it).
Open question (info, not a defect) — readCandidate uses os.ReadFile (housestyle.go:149), which follows symlinks, so a repo could point HIVE_STYLE.md at a host file and have up to style_max_bytes of it land in every kick. This is the same posture AGENTS.md already has (src/crux/agentsmd/agentsmd.go:203), so it is not a regression; noting it because this PR adds a second repo-controlled → prompt channel. An os.Lstat + ModeSymlink refusal would close it cheaply if the owner wants that.
Not approving (human signature), but I see nothing blocking.
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Read pkg/housestyle/housestyle.go, pkg/scheduler/kick_housestyle.go, the assembleKick seam list, and the AGENTS.md priming path it sits beside. The precedence, cap and fall-through logic match the description: Resolve (housestyle.go:113-150) skips an oversize or scanner-blocked repo candidate whole and falls to inline → file → Builtin, never clipping; readCandidate (housestyle.go:155-171) is silent only on ErrNotExist, so unreadable files do surface as skips. addHouseStyle is the first entry in staticSeams (kick_layout.go:83) and the seams are applied in reverse, so it does land directly under the kick header, which is also why SourceFromKick's first-match strings.Index (housestyle.go:183) cannot be spoofed by a later issue body containing the same header text. The scan hook uses the real ioscan.ScanInputFrom(..., SurfaceIssueBody, policy) API (ioscan/policy.go:179). Looks correct to me.
One low-severity note on the trust boundary the PR itself draws:
housestyle.go:156—os.ReadFile(RepoRoot/HIVE_STYLE.md)follows symlinks, so the "untrusted" repo file can point outside the checkout. AHIVE_STYLE.md -> ../../some/host/filecommitted to the managed repo makes the scheduler (not the agent) read up tostyle_max_bytesof any file it can see and place it at the top of every kick.ioscanat the issue-body policy will catch credential-shaped text, but not arbitrary host content. This is parity with the existing AGENTS.md read (crux/agentsmd/agentsmd.go:203, also plainos.ReadFile, and that one is not scanned at all), so it is not a regression — but since this PR is the one declaring the file untrusted and adding a scan, anos.Lstat+ "refuse symlink / refuse non-regular file" check inreadCandidateforSourceRepowould close it cheaply and give the audit line a clear reason. Suggestion, not a blocker.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
merged by Hive trusted-author auto-merge: author @clubanderson holds role owner and GitHub permission admin on repo; CI green at 6137ba4. |
|
Thank you for your contribution! Your PR has been merged. We'd love to hear how your experience was: share feedback |
Fixes #14575
What
Every kick, on every backend, now opens with a HOUSE STYLE section — how changes are to be made (smallest complete diff, follow existing patterns, tests for behaviour changes, keep names stable, DCO) — as distinct from what to work on (the kick body) and the repo's own rules (
AGENTS.md).Precedence: repo
HIVE_STYLE.md(read from theproject.checkouts_dircheckout, same root asAGENTS.md) > hive default (project.styleinline, elseproject.style_file) > built-in.Safety
project.style_max_bytes(default 4096, min 512) and skipped whole when oversize — the next source applies; never clipped.pkg/ioscanat the issue-body policy and refused whole on a block; falls through to hive/built-in. Operator-supplied text is trusted. Every skip writes ahouse_style_skipaudit line (agent, source, path, reason); ioscan blocks also write their usualioscan_blocklines.TestKickLayoutStaticPrefixIsCacheableupdated to assert it leads the static prefix).Dashboard
Agent card and detail pane show a style field (
repo/hive/built-in), read back from the kick'sHOUSE STYLE (source: …)header — so it reports what was actually sent./api/statusexposesagents[].styleSource. CSS uses existing tokens only.Docs
New
docs/house-style.md;operator-reference.mdrows forproject.style,style_file,style_max_bytes;agents-md.mdanddocumentation-map.mdcross-links;hive.yaml.example;api-reference.md; changelog fragment.Tests
Non-goals (follow-ups noted on the issue)
HIVE_STYLE.mdwhen there is no checkout — same posture asAGENTS.md.Evidence (screenshots)
Captured against hive's own spoke (hive-oke) via a local proxy serving this branch's SPA; the deployed build predates
styleSource, so the proxy injects the precedence outcome a hive with a repoHIVE_STYLE.mdreports.Agent card — new style field (
repo):Agent detail pane — STYLE field:
Tests:
go test ./pkg/housestyle ./pkg/scheduler ./pkg/dashboard/...green locally (see PR checks).