feat: check the map body's two stated budgets - #452
Merged
Merged
Conversation
Adds scripts/map-budget.sh, checking #247's body against its own stated 150,000-byte total budget and 400-byte-per-entry cap on "Decisions so far", plus scripts/map-budget-test.sh pinning its 0/1/2 exit contract offline. Graduated from #442's rewrite, which stated the numbers but left them unchecked. pg-core/tests/ci_wiring.rs gets one new assertion that build.yml's ruleset-drift job runs the checker's self-test. It is RED on this branch: dobby-coder has no workflows: write, so the build.yml step and the new map-budget.yml live check are posted on #445 for a maintainer to apply, not pushed here. Part of #247. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
dobby-coder
Bot
changed the title
![dobby-coder[bot]](https://avatars.githubusercontent.com/in/3365457?s=60&v=4){kind=small}
Contributor
Author
There was a problem hiding this comment.
Verdict: request-changes (posted as a COMMENT review — GitHub does not allow REQUEST_CHANGES/APPROVE on a PR authored by this same dobby-coder identity, so the event type below is a technical fallback, not the actual verdict).
Rule check: 5 rules bound this task (atomic-commits, code-comments, draft-pull-requests, no-summary-issues, test-suite-before-submit) — all compliant. Single well-described commit, comment density in the two new scripts matches their established neighbours (scripts/ruleset-drift.sh, scripts/wasm-package-check.sh), the PR is a draft, no summary issue was created, and the full pg-core suite plus fmt/clippy ran clean apart from the one documented, expected-red ci_wiring assertion.
Review findings: one bug in the committed script, one bug in the posted-but-not-committed map-budget.yml patch (can't be fixed by pushing here — no workflows: write — but the patch text itself, posted in this PR's description and on #445, can and should be revised before a maintainer applies it), and one non-blocking nit.
.github/workflows/map-budget.yml(posted as a patch in the PR description / on #445, not part of this diff): its dedup step readsgh api repos/$GH_REPO/issues/$ISSUE_NUMBER/comments --jq '.[-1].body // ""'with no--paginate/per_page, so it only ever sees the API's first page (30 comments). #247 already has 26 comments and is the project's active running map; once it crosses 30,.[-1]silently stops being the true most-recent comment and the dedup this step exists for breaks, reintroducing the edit-storm spam it was built to prevent. Please revise the posted patch (in the PR body and the #445 comment) to paginate or useper_page+ a proper "last page" fetch before a maintainer applies it.
The scripts/map-budget.sh comment/constant mismatch below is the blocking finding; the nit on ci_wiring.rs is informational only.
| set -euo pipefail | ||
|
|
||
| # 150,000 bytes, stated in #247's "## Notes" header comment. | ||
| readonly BODY_BUDGET_BYTES=150000 |
Contributor
Author
There was a problem hiding this comment.
BODY_BUDGET_BYTES=150000's comment (line 48) claims "150,000 bytes, stated in #247's own 'Notes' header comment" — but the header comment (gh issue view 247 --json body) only ever says "keep the body under 150 KB", never an exact byte figure. That same header sentence states GitHub's hard cap as an exact 262,144 bytes (= 256*1024), i.e. binary-KB math; under that same convention 150 KB reads as 153,600 bytes, 3,600 more than this constant. A body sized 150,000–153,600 bytes would be flagged over-budget here despite plausibly being within the issue's actual intended budget. Either fix the comment to stop claiming the header states an exact byte count (it's this script's own decimal-KB interpretation), or use 153600 if binary was intended. The same ambiguity applies to the out-of-scope 16,000-byte "Notes" figure mentioned at line 20 of this file.
| fn the_map_budget_checkers_self_test_runs_in_ci() { | ||
| let job = job(&workflow(BUILD_WORKFLOW), "ruleset-drift", BUILD_WORKFLOW); | ||
| let steps = steps(&job); | ||
|
|
Contributor
Author
There was a problem hiding this comment.
Nit, not blocking: the_map_budget_checkers_self_test_runs_in_ci is the one cargo-test failure (18/19 pass) — fully expected and documented in both the test's own doc comment and the PR description: dobby-coder can't push .github/workflows/*.yml, so the build.yml step this asserts on is posted as a patch instead of committed. Needs a maintainer to apply the posted build.yml patch; no action needed here.
…header #247's "## Notes" header comment says "keep the body under 150 KB", not an exact byte figure, but that same sentence states GitHub's hard cap as an exact binary-KB conversion (262,144 = 256*1024). Reading "150 KB" the same way gives 153,600 bytes, not the 150,000 this script assumed -- a body sized 150,000-153,600 bytes was being flagged over budget despite being within the issue's actual intended budget. Also fixes the comment that overclaimed the header states an exact byte count. Found in review of #452.
Contributor
Author
|
Workflow handover (1/2):
The change is four lines appended to the end of the Full contents of
|
Contributor
Author
|
Workflow handover (2/2): new The live check (#445): a new workflow, gated on the Full contents of new
|
Contributor
Author
There was a problem hiding this comment.
Rule-compliance check against ~/dobby-rules.md (5 binding rules: atomic-commits, code-comments, draft-pull-requests, no-summary-issues, test-suite-before-submit) found no violations — the PR is a draft, its two commits are atomic and well-described (the second is a documented fix-up found in review), the full pg-core test suite plus fmt/clippy/offline self-tests were run and reported, no summary/report issues were created, and comment density in the new scripts matches its siblings (ruleset-drift.sh, changelog-coverage.sh) and ci_wiring.rs's existing assertions.
One bug survives from review: scripts/map-budget.sh's heading-existence check and its section-extraction disagree on what counts as the ## Decisions so far heading, which lets a heading-level typo silently defeat the entry budget check — exactly the case the script's own header disclaims ("Never reported as 0 -- a body that cannot be parsed is not a body within budget"). See inline comment for a reproduction and a verified one-line fix that keeps the existing 12/12 map-budget-test.sh suite green.
Verdict: changes requested (posted as a COMMENT review, not REQUEST_CHANGES — GitHub rejects that event on a PR authored by this same bot identity). That one fix is needed before this is ready to apply the posted workflow patches; leaving in draft.
| exit 2 | ||
| fi | ||
|
|
||
| if ! grep -qF '## Decisions so far' "$path"; then |
Contributor
Author
There was a problem hiding this comment.
This existence check and the section-extractor just below it (line 84, /^## Decisions so far/) disagree on what counts as the heading. grep -qF here is unanchored, so it matches ## Decisions so far as a substring anywhere in a line — including inside a heading-level typo like ### Decisions so far (three #s: drop the leading # and the rest is an exact substring match). But the awk extractor's ^## Decisions so far is anchored, and a three-# line never matches it, so found stays 0 and section comes back empty. Every entry inside that (mistyped) section is then silently skipped by the byte-cap loop below, and the script falls through to reporting exit 0 "OK" even when an entry in the section is well over the 400-byte cap — the exact case the module comment above disclaims ("Never reported as 0 -- a body that cannot be parsed is not a body within budget").
Reproduced: a body with a ### Decisions so far heading and a 450-byte entry inside it exits 0 today. Anchoring the existence check the same way as the awk extraction fixes it — the same input then correctly exits 2 (heading not found), and the full map-budget-test.sh suite (12/12) still passes unmodified.
Suggested change
| if ! grep -qF '## Decisions so far' "$path"; then | |
| if ! grep -qE '^## Decisions so far' "$path"; then |
grep -qF '## Decisions so far' matched the string anywhere in a line, including inside a heading-level typo like '### Decisions so far'. The awk section-extractor right below it is anchored (^## Decisions so far), so that typo left `found` unset there: the section came back empty, every entry inside it was silently skipped, and the script fell through to exit 0 "OK" -- the exact "reported as 0 when it can't parse" case the module comment disclaims. Anchor the existence check the same way as the extractor. Adds a committed regression case to map-budget-test.sh: a '### Decisions so far' typo with a 450-byte entry inside it now exits 2, not 0; the existing 12 cases are unaffected. Found in review of PR #452.
Contributor
Author
There was a problem hiding this comment.
Verdict: APPROVE (posted as COMMENT — GitHub blocks self-approval; this PR's author is app/dobby-coder, the same identity as this review)
Rule check (5 binding rules) and review findings both clean.
Rule compliance (~/dobby-rules.md):
atomic-commits— three focused commits: add the checker + tests, fix the KB-budget arithmetic, fix the heading-anchor bug with a regression test.code-comments— comment density inci_wiring.rsandmap-budget.shmatches their established siblings; no restated-obvious comments found.draft-pull-requests— PR is open as a draft.no-summary-issues— no issues created by dobby-coder; only comments on the pre-existing tracking issue (#445) and this PR carrying the workflow-patch handover.test-suite-before-submit—scripts/map-budget-test.sh(13/13) andcargo test/fmt/clippyresults are documented in the PR description, with the one expected-red assertion (the_map_budget_checkers_self_test_runs_in_ci) explained and pre-existing.
Review findings (from the prior review cycle, verified at head e1710db): the grep -qF → grep -qE '^## Decisions so far' fix correctly aligns the heading-existence check with the awk section extractor's anchor, closing the heading-typo false-pass. The new regression test is well-built and the full suite passes. CI red is confirmed pre-existing and out of this container's reach (no workflows: write), documented both in-code and in the PR body; the other three Test workspace failures are matrix cancellations cascading from that one red job, not independent findings.
No blocking issues. Staying in draft per the two workflow patches awaiting a maintainer to apply them (posted on #445 and on this PR).
) Applies the two workflow patches posted on #445, which dobby-coder cannot push without workflows: write: - build.yml: a 'Test the map-budget checker' step in the ruleset-drift job, which turns the_map_budget_checkers_self_test_runs_in_ci green. - map-budget.yml: checks the wayfinder:map issue body on every edit; comments on exit 1 (deduped against the latest comment), ::error:: on exit 2.
rubenhensen
approved these changes
Sep 23, 2026
Merged
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.
What this does
Implements #445: two checks on issue #247's stated budgets, which #442
rewrote and capped but left unenforced.
scripts/map-budget.sh <path-to-body-file>: checks a body file againstthe 153,600-byte (150 KiB) total budget (
## Notes) and the 400-byteper-entry cap on
## Decisions so far(that section's own headercomment). Both are UTF-8 byte counts. Exit 0 means within budget, 1 a
real finding, 2 could not determine; the three are never conflated.
scripts/map-budget-test.sh: an offline self-test, 13 cases, includingthe em-dash case (400 characters, over 400 bytes), a heading-level typo
that must not satisfy the section check, and a permanent known-good
fixture.
scripts/testdata/map-247-body.md: a byte-for-byte snapshot of Wayfinder: fleet remediation — enforce the seams, kill silent failure (2026-07 audit) #247'sreal body, taken via
gh issue view 247 -R encryption4all/postguard --json body -q .bodyon 2026-09-23 (68,356 bytes). A snapshot, not alive copy.
pg-core/tests/ci_wiring.rs,the_map_budget_checkers_self_test_runs_in_ci, modeled onthe_wasm_package_checkers_self_test_runs_in_ciper the issue'spre-flight amendment.
Why this assertion is RED on this branch
dobby-coderhas noworkflows: write, so it cannot push under.github/workflows/. The newci_wiring.rsassertion expectsbuild.yml'sruleset-driftjob to runscripts/map-budget-test.shalongside its four existing self-tests; that step is posted below for a
maintainer to apply, not pushed here. This is deliberate, not an oversight:
see the issue's §3 and the test's own doc comment.
git diff --name-only origin/main...HEAD -- .github/workflows/is empty onthis branch. Confirmed nothing else regressed: every other check on this PR
is green except the four cascading from this one via the
testjob'smatrix (no
fail-fast: false) --Test workspace (pg-ffi),(pg-cli),(pg-pkg)and(cryptify)all showcancelledat the API level, not asecond failure. Nothing here is fixable from this container; this is the
"cannot be fixed from here" case, not an oversight left unaddressed.
This PR's two comments below are a ready-to-apply handover, not just the
diffs in this description: each posts the full resulting file (so a
maintainer reviews exactly what will run with the repo's secrets, not a
diff) followed by one
gh api -X PUTcommand that computes its own currentsha and needs no checkout to run.
CI on this PR
Test workspace (pg-core)fails on exactly the one assertion above (18/19pass).
build.yml'stestjob is acratematrix with nofail-fast: false, so that one failure cancels its still-running matrix siblings.Test workspace (cryptify),Test workspace (pg-cli)andTest workspace (pg-pkg)show red/cancelled as a result, not because of anything they ranthemselves; the same cancellation pattern is present on the pre-title-fix
run too, so it is not something this push introduced.
Wire compat, theruleset's one required check, is green. The PR title also failed
Conventional Commiton open (no type prefix) and has since beenretitled; that check is green now.
Revised since review
Three findings from review of this PR.
Two fixed in 1295b8a:
scripts/map-budget.sh'sBODY_BUDGET_BYTESwas150000(decimal-KBmath), while its own comment claimed that figure came from Wayfinder: fleet remediation — enforce the seams, kill silent failure (2026-07 audit) #247's
header. The header only says "keep the body under 150 KB", never an
exact byte count -- and that same sentence states GitHub's hard cap as
an exact binary-KB conversion (
262,144=256*1024). Read the sameway, "150 KB" is
153,600bytes, not150,000-- a body sized150,000-153,600bytes was being flagged over budget despite beingwithin the issue's actual intended budget. Constant, comment,
scripts/map-budget-test.sh, and this description all updated tomatch.
workflows: write)map-budget.ymlpatch's dedup step read
gh api repos/.../comments --jq '.[-1].body'with no pagination, so once Wayfinder: fleet remediation — enforce the seams, kill silent failure (2026-07 audit) #247 crosses the API's default 30-comment
page size the dedup silently stops seeing the true most recent comment,
reintroducing the edit-storm spam it exists to prevent. The patch below
now paginates and round-trips the comment body through base64 -- a
plain
tail -n 1on unencoded, multi-linejqoutput would grab onlythe last line of the last page's comment, not the whole last comment.
Verified locally against a stubbed
ghreturning a multi-page,multi-line response. Also revised in the matching patch comment on
check the map body's two stated budgets: 150 KB total, 400 bytes per index entry #445.
Fixed in e1710db:
scripts/map-budget.sh's heading-existence check (grep -qF '## Decisions so far') and the awk section-extractor just below it (^## Decisions so far) disagreed on what counts as the heading: the existence check isunanchored, so it matched the string anywhere in a line, including inside
a heading-level typo like
### Decisions so far; the extractor isanchored and never matches that line, so the section came back empty and
every entry inside it was silently skipped. A body with that typo and a
450-byte entry inside it exited 0 "OK" -- the exact "reported as 0 when
it can't parse" case the module comment disclaims. Anchored the existence
check the same way as the extractor (
grep -qE '^## Decisions so far').Added a committed regression case to
scripts/map-budget-test.sh; theexisting 12 cases are unaffected.
The two workflow patches (posted on #445, not committed here)
Both are also posted as a comment on the issue, per §4 of the ticket, and as
two ready-to-apply comments on this PR itself (full file + one-paste
gh apicommand each):
build.yml,new
map-budget.yml.(a)
build.yml: adds the self-test step toruleset-drift, after theexisting five.
(b) A new
.github/workflows/map-budget.yml: the live check, gated onthe
wayfinder:maplabel, triggered onissues: edited(plusworkflow_dispatch), never apush. Writesgithub.event.issue.bodyto afile via
env:, never interpolated intorun:. Runsscripts/map-budget.shas a bare command; on exit 1 comments on the issue(deduped against the most recent comment); on exit 2 prints
::error::andcomments nothing. Not wired into any required context.
Verification
bash scripts/map-budget-test.sh: 13/13 pass, offline, noGH_TOKEN.cargo test --manifest-path pg-core/Cargo.toml --features test,rust,stream:91 passed, 1 failed (
the_map_budget_checkers_self_test_runs_in_ci,the expected RED assertion).
cargo fmt --manifest-path pg-core/Cargo.toml --all -- --check: clean.cargo clippy --manifest-path pg-core/Cargo.toml --all-targets --features test,rust,stream -- -D warnings: clean. The bare--all-targetscommand from the issue's acceptance list needs the same
test,rust,streamfeatures the test command uses; that gap is pre-existing on
origin/mainand unrelated to this change.
scripts/map-budget.sh scripts/testdata/map-247-body.mdexits 0.+90,000bytes → exit 1, names158356against the153600budget.- [entry inside the section → exit 1, names401andthe entry text.
## Not yet specified)→ exit 0.
## Decisions so farheading deleted → exit 2.## Decisions so farreplaced with### Decisions so far(heading-leveltypo) plus a 450-byte entry inside it → exit 2, not the 0 it returned
before e1710db.
ci_wiring.rsassertion itself: applied thebuild.ymlpatch above locally (uncommitted) →cargo test --test ci_wiringgoes green (19/19); deleted just the new step → reds again onexactly
the_map_budget_checkers_self_test_runs_in_ci; reverted the localedit entirely (
git checkout -- .github/workflows/build.yml).gh: withinbudget → exit 0, no
ghcalls; over budget → exit 1, comment posted;over budget with an identical most-recent comment → exit 1, no repost;
undetermined → exit 2,
::error::, noghcalls at all.Part of #247.
🤖 Generated with Claude Code