Repository navigation
docs(agents): drop the commit and push gate, and require screenshots on UI changes - #757
Merged
Merged
Conversation
Agents may now commit, push a branch and open a PR without asking. The surviving guards are branch protection on main and the owner's merge, so nothing reaches main unreviewed either way. The rule carried no decision-record link, so by this file's own preamble it was a plain convention rather than an earned rule. In practice it was also ambiguous about whether the gate was every commit or only the push, which cost a round of clarification mid-slice on #721.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Broadens the #662 rule from a PR whose PURPOSE is visual to any PR that changes what a user sees, since the trigger is what a reader can no longer check from the diff. Adds the before/after expectation and the attachment mechanic, and a pointer from the PR-workflow section where people actually look while opening one.
mforce
changed the title
mforce
added a commit
that referenced
this pull request
Sep 11, 2026
…rder (#756) Closes #721 Epic #719, slice 5. Confirming a sales order with any line priced below its snapshotted list price now requires a reason, chosen from a closed picklist with an optional note that becomes required for *Other*. The reason is stored on the order and surfaced on the order panel, in the Orders list and in the `sales-orders` CSV export. Design record: `docs/plans/721-discount-reason/01-design.md`. ## What decides "discounted" `SalesOrder.HasBelowListLine` — a line counts only when a comparable list price exists and the sale price is strictly under it. That answers two of the epic's open questions: a `NULL` list price is not a discount (nothing to compare against), and a price **above** list is not one either. Same two branches the SPA's `lineDiscount` already uses, so the screen and the server cannot disagree. The reason is **per order**, not per line (epic open question 2). Per line multiplies clicks on the app's most routine write and buys precision no consumer — #725, #726, #728 — asks for. ## Two corrections the work made to its own design **The optional body does not bind the way the design claimed.** The design cited `DisableUser` as proof that a bodyless POST still reaches an endpoint with a nullable body parameter. It proves the opposite: `DisableUserTests.Disable_WithNoContentTypeAtAll_Is404_NotUnsupportedMediaType` pins that exact 404. A typed body parameter attaches `application/json` Accepts metadata, the consumes matcher turns it into a route constraint, and a POST with no `Content-Type` — which is what every existing caller sends — stops matching and falls through to `Program.cs`'s `/api/{**rest}` catch-all. Four tests went red the moment the parameter landed. The fix is one line of route metadata, `.Accepts<ConfirmSaleRequest>(isOptional: true, "application/json", "*/*")`, which changes no caller; `Confirm_WithNoContentTypeAtAll_StillConfirms` pins it. **The rules belong in `CheckCanConfirm`, not in `Confirm`.** The design put them in `Confirm`, which meant a missing reason was refused only after `ConfirmSaleHandler` had planned and applied a whole FIFO allocation — and the handler's "contradicted its own CheckCanConfirm" throw then turned that 422 into a 500. Both corrections are written back into the design record. ## #394 — the callers CI does not cover Read from source against the seeded catalog, not inferred from a green run. - `tools/simulation/ui/specs/worker-sale-allocation.spec.ts` **broke.** It prices a line at `0.01` against `Sim Large Eggs` at `45`, so after this change the first click opens the reason dialog, the spec's wait for `sales:confirmOrderTitle` never resolves, no `/confirm` POST is sent, and its `waitForResponse` can never settle. Fixed here by driving the new dialog while keeping `0.01` and every #612 assertion, so it now covers both paths. - `DemoDataSeeder` and `SimulationDataSeeder` are unaffected: both add lines with a `null` unit price, so the handler prices them at list. The simulation seeder *does* seed below-list lines via `EnsureExtraLineAsync`, but only onto drafts that are never confirmed — a near miss worth knowing about before moving them. - `tools/simulation/k6/` never confirms a sale at all (`bundles.js:391-393`). - `tools/simulation/ui/specs/sales.spec.ts` survives by five minor units — it sells at `0.50` against a `45` list — and now says so in a comment. ## Verification **Driver-verified** (run and read by the reviewing agent, on the final source state, not quoted from the implementer): ``` Passed! - Failed: 0, Passed: 428, Skipped: 0, Total: 428 - Cluckwork.Domain.Tests.dll Passed! - Failed: 0, Passed: 10, Skipped: 0, Total: 10 - Cluckwork.AppHost.Tests.dll Passed! - Failed: 0, Passed: 281, Skipped: 0, Total: 281 - Cluckwork.Application.Tests.dll Passed! - Failed: 0, Passed: 1743, Skipped: 0, Total: 1743 - Cluckwork.Api.IntegrationTests.dll Test Files 122 passed (122) Tests 2810 passed (2810) ``` `dotnet build Cluckwork.sln` is 0 warnings / 0 errors, `tsc -b --noEmit` is clean, and `tools/schema-docs/generate.sh --check` reports `docs/schema/ is up to date` (#417). **One mutation planted and observed by the driver**, rather than trusting the implementer's report: flipping `HasBelowListLine`'s strict `<` to `<=` turns four domain tests red; restored, re-run, 428 pass and the file diffs clean. **Implementer-attested, not driver-verified:** 39 further mutations across the domain rules, the API surface, the SPA and the new guards, all reported red with none surviving. `git grep MUTANT` returns only the repo's own pre-existing Playwright harness. **CI-verified** (corrects an earlier claim in this body): the `Playwright smoke over the simulation fixture` job passed in 4m8s against a real seeded stack, so both specs were exercised — including `worker-sale-allocation.spec.ts`, the one this change broke and repairs here. An earlier revision said neither suite had been run by anyone; that was written before the job reported and was wrong. **Still owed:** - **Screenshots.** This slice adds a dialog, so it owes a before/after capture under the rule broadened in #757. Blocked on a local dev admin credential, not on the work. **Reviewer status.** CodeRabbit did not review this PR. Its check reports `pass`, but the detail reads `Review skipped: manual review required for this OSS repository` — the green badge here means no review ran, not that nothing was found. Recorded so the silence is not read as a clean pass. ## Every acceptance criterion is met The reason survives to **order detail**, the **Orders list**, the **`sales-orders` CSV export** and, as of `c364f93`, **history** — the `SalesOrder.Confirm` audit row carries `discountReasonCode` and `discountReasonNote`, and the Audit page's Details column renders them. An earlier revision of this body argued for leaving history out, on the grounds that a copy in the audit row would be a second source of the same truth. The owner reversed it, and the reversal is right: the reason is write-once and the audit row commits in the **same transaction** as the columns, so the two cannot disagree. The duplication risk that argument guarded against does not exist. The design record's non-goal is struck through with that reasoning rather than quietly rewritten. Two behaviours worth knowing. An order confirmed at list writes **no payload at all** rather than a payload of nulls. And there is **no backfill**: an order confirmed before this shipped shows an em dash in Details even where its columns carry a reason, which is the honest rendering of "recorded somewhere this view cannot reach". --------- Co-authored-by: mforce <cleyva@clvc.net> Co-authored-by: mforce <mforce@users.noreply.github.com>
mforce
pushed a commit
that referenced
this pull request
Sep 12, 2026
🤖 I have created a release *beep* *boop* --- ## [0.1.0](v0.0.4...v0.1.0) (2026-09-12) ### ⚠ BREAKING CHANGES * log in by farm code, with per-account email identity ([#532](#532)) (#564) ### Features * **accounts:** add Account.Slug (farm code), suspend/reactivate, list-accounts verb ([#531](#531)) ([3fe9754](3fe9754)) * **accounts:** provision additional farms ([#581](#581)) ([006f298](006f298)) * add Aspire local development AppHost ([#567](#567)) ([2c9e6b9](2c9e6b9)) * add configurable worker sale allocation ([#619](#619)) ([0955095](0955095)) * add searchable entity pickers ([#642](#642)) ([60d2053](60d2053)) * **api:** provision-account takes an optional --timezone at creation ([#603](#603)) ([#694](#694)) ([a0aee39](a0aee39)) * **audit:** show the sales-line audit payload as a readable Details column ([#745](#745)) ([#749](#749)) ([d26d389](d26d389)) * **auth:** add ApplicationUser.StepUpLogoutEpoch column ([#338](#338)) ([#554](#554)) ([18306ee](18306ee)) * certify over-cap simulation fixture bands ([#633](#633)) ([a67b2e1](a67b2e1)), closes [#627](#627) * **cli:** rename-account verb to change a farm code ([#732](#732)) ([#733](#733)) ([4b70559](4b70559)) * **customers:** edit existing customer details ([#625](#625)) ([#626](#626)) ([062a55c](062a55c)) * **jobs:** single-runner leader gate for the durable job worker ([#271](#271)) ([#555](#555)) ([4148f9b](4148f9b)) * let owners change user email addresses ([#605](#605)) ([842347b](842347b)) * log in by farm code, with per-account email identity ([#532](#532)) ([#564](#564)) ([68adb62](68adb62)) * **ratelimit:** distributed IP-keyed auth limiters ([#544](#544)) ([#558](#558)) ([ec14972](ec14972)) * **ratelimit:** distributed per-account report concurrency cap with local-ceiling fallback ([#545](#545)) ([#559](#559)) ([1522e4e](1522e4e)) * **sales:** mark discounted lines, total the discount, and show it in the Orders list ([#723](#723), [#724](#724)) ([#741](#741)) ([1a07441](1a07441)) * **sales:** record list, old and new price in the order-line audit payload ([#722](#722)) ([#742](#742)) ([97c866f](97c866f)) * **sales:** refuse an over-ceiling confirm from a Sales user ([#727](#727)) ([#766](#766)) ([8c0792a](8c0792a)) * **sales:** show what each order still owes, and filter the list to unpaid ([#771](#771)) ([ca59d68](ca59d68)) * **sales:** snapshot the list price on the order line and show the discount ([#734](#734)) ([cffed5e](cffed5e)) * **sales:** snapshot the product name and unit in the order-line audit payload ([#747](#747)) ([#748](#748)) ([0481c06](0481c06)) * scope Worker reads to assigned flocks ([#388](#388)) ([#611](#611)) ([5884a9a](5884a9a)) * shared-state ports with Redis + in-process fallback ([#543](#543)) ([#552](#552)) ([f767fa9](f767fa9)) * suspend-account / reactivate-account operator verbs ([#534](#534)) ([#573](#573)) ([d0be26c](d0be26c)) * **tenancy:** write-side tenant guard + single-assignment TenantContext ([#546](#546)) ([#561](#561)) ([f371f1d](f371f1d)) * **web:** dashboard rework — capture-status tiles, 14-day trend, stock as a stacked bar ([#654](#654)) ([396ba23](396ba23)) * **web:** date-range filters on audit and expenses, and the stock lot filter gets its bounded toolbar ([#666](#666), [#667](#667), [#653](#653)) ([94b188f](94b188f)) * **web:** elevation hierarchy and sentence-case labels ([#651](#651), [#652](#652)) ([#661](#661)) ([28db4c7](28db4c7)) * **web:** Expenses and Audit keep a clear-filters control while rows are still showing ([#679](#679)) ([#697](#697)) ([b859982](b859982)) * **web:** expenses filters by a date range like its sibling screens ([#667](#667)) ([f13858f](f13858f)) * **web:** key the farm brand palette per farm ([#586](#586)) ([#600](#600)) ([7183a43](7183a43)) * **web:** let operators forget remembered farms ([#598](#598)) ([577d94e](577d94e)) * **web:** one-line provenance, bounded date filters, and empty states that invite action ([#653](#653), [#655](#655)) ([#668](#668)) ([80b53f4](80b53f4)) * **web:** prefill the farm code from ?farm= and remember it ([#535](#535)) ([#588](#588)) ([b7f5cc6](b7f5cc6)) * **web:** split authenticated routes into lazy chunks ([#620](#620)) ([5089271](5089271)) * **web:** the audit log filters by a date range, and says which window is empty ([#666](#666)) ([63027e0](63027e0)) * **web:** typeset numbers as numbers and refresh the Help glossary ([#650](#650), [#657](#657)) ([af4fe11](af4fe11)) ### Bug fixes * **api:** order same-instant audit events by a durable monotonic key ([#700](#700)) ([8fcf084](8fcf084)) * **api:** print the farm code from bootstrap-admin ([#589](#589)) ([#594](#594)) ([34032ac](34032ac)) * **audit:** show the price a line sold for, not its list price ([#759](#759)) ([e6b37d0](e6b37d0)) * **audit:** store catalog enums by name and guard the add-item transaction shape ([#751](#751)) ([23609ff](23609ff)) * **auth:** reject invalid account claims ([#622](#622)) ([8d6c7fe](8d6c7fe)) * **auth:** require step-up for durable user access ([#360](#360)) ([#607](#607)) ([f767dce](f767dce)) * **ci:** bound the npm audit calls and give the web job room to finish ([#686](#686)) ([153b7a8](153b7a8)) * **ci:** escalate the audit bound to SIGKILL, so it actually bounds ([#686](#686)) ([a0c8f4e](a0c8f4e)) * **ci:** fail closed on invalid vulnerability config ([#621](#621)) ([1690db8](1690db8)) * **ci:** lockfix covers the two AppHost lock files, derived from the sln ([efb05e6](efb05e6)) * **ci:** lockfix covers the two AppHost lock files, derived from the sln ([8986d77](8986d77)) * **ci:** remove invalid XML comment from nuget.lockfix.config ([#541](#541)) ([5f1bc0a](5f1bc0a)) * **ci:** the advisory vuln gate no longer blocks on an unusable report ([#686](#686)) ([aaf6934](aaf6934)) * **ci:** the advisory vuln gate no longer blocks on an unusable report ([#686](#686)) ([64f1f53](64f1f53)) * **i18n:** tl help text names the saleable flag and unit-system setting what their labels call them ([#688](#688)) ([#696](#696)) ([bfd24d7](bfd24d7)) * **infra:** AccountId must be a non-nullable Guid or both tenant write layers refuse ([#673](#673)) ([#695](#695)) ([2470c4e](2470c4e)) * require step-up for flock scope changes ([#609](#609)) ([4151f89](4151f89)) * **sales:** keep a line's discount markers agreeing while its price is edited ([#752](#752)) ([#753](#753)) ([c159b4b](c159b4b)) * **sales:** say which kind of missing list price a line has ([#774](#774)) ([489180e](489180e)) * scope legacy logout to selected farm ([#624](#624)) ([fae8d82](fae8d82)) * **seed:** drain the daily-entry lock sweep so deep simulation fixtures validate ([#644](#644)) ([730fa23](730fa23)), closes [#638](#638) * **tenancy:** AccountId is a concurrency token, so the database refuses a detached cross-tenant write ([#562](#562)) ([4d1dfa3](4d1dfa3)) * **tenancy:** AspNetUserRoles carries a tenant column, so a role write naming another farm's user is refused ([#670](#670)) ([fc0552a](fc0552a)) * **tests:** bump the image-pin allow-list counts for the AppHost LocalPorts tests ([#593](#593)) ([58d3056](58d3056)) * **tests:** the OTLP collector survives a lost port race and ignores traffic that is not an export ([#672](#672), [#676](#676)) ([#677](#677)) ([965c737](965c737)) * **web:** a scoped audit view filtered to nothing names both the record and the range ([#666](#666)) ([41bbfe1](41bbfe1)) * **web:** an abandoned dialog attempt's success no longer hijacks the replacement on Customers, Daily Entry, Flocks, Grades and Products ([#703](#703)) ([#705](#705)) ([85605db](85605db)) * **web:** an abandoned dialog attempt's success no longer hijacks the replacement on Inventory, Expenses, History and Stock ([#703](#703)) ([#706](#706)) ([60a4997](60a4997)) * **web:** an abandoned edit's success no longer hijacks the dialog that replaced it on Users ([#703](#703)) ([#710](#710)) ([778faab](778faab)) * **web:** an abandoned order attempt's success no longer hijacks the dialog that replaced it ([#702](#702)) ([522c699](522c699)) * **web:** capture screens open on the flock you last used, and assigning one no longer guesses ([#646](#646)) ([#699](#699)) ([7f8f317](7f8f317)) * **web:** constrain dialog session helpers to declared scopes ([#715](#715)) ([389e3c8](389e3c8)) * **web:** date validation gets one boundary table instead of one case per review round ([#666](#666)) ([215f830](215f830)) * **web:** keep a paged window and an item panel on the user's newest intent ([#645](#645)) ([d81bccf](d81bccf)) * **web:** keep Sales order panels closed after pending writes ([#711](#711)) ([f0f7492](f0f7492)) * **web:** keep Sales panels closed after pending Open reads ([#716](#716)) ([620411f](620411f)) * **web:** make login take the cross-tab cookie lock so a racing refresh cannot restore the wrong session ([#648](#648)) ([ff18beb](ff18beb)) * **web:** make the entity picker read as a search field and focus it on open ([#736](#736)) ([66ef667](66ef667)), closes [#735](#735) * **web:** page truncated customer and movement tables with usePagedList ([7cfe4d6](7cfe4d6)) * **web:** reconcile Sales line edits with refreshed orders ([#717](#717)) ([d7dd2c9](d7dd2c9)) * **web:** the audit date filter accepts low-numbered years, and its empty state covers every narrowing ([#666](#666)) ([af52d25](af52d25)) * **web:** the audit date filter rejects impossible dates, and its history guard actually guards ([#666](#666)) ([8d51846](8d51846)) * **web:** the expense range bounds are not capped at today, which the month-end default exceeds ([#667](#667)) ([7e01864](7e01864)) * **web:** the help text calls the expiry field what the field calls itself ([#666](#666)) ([2fd1f3c](2fd1f3c)) * **web:** the stock lot date range sits in the bounded toolbar ([#653](#653)) ([43dec5e](43dec5e)) ### Refactoring * **web:** extract SalesPage's dialog-write wrapper into a shared useDialogAction hook ([#703](#703)) ([#704](#704)) ([60ee9d9](60ee9d9)) ### Documentation * add k6 preparation steps to the dev-database fixture runbook ([#643](#643)) ([a4f1f09](a4f1f09)) * add runbook for loading the simulation fixture into a dev database ([#639](#639)) ([2d143b8](2d143b8)) * **agents:** a PR closes its issue from the body, not the title ([#744](#744)) ([39be13c](39be13c)) * **agents:** drop the commit and push gate, and require screenshots on UI changes ([#757](#757)) ([6225172](6225172)) * **agents:** find guards by grepping registry readers; amend issues a PR overtakes ([#580](#580)) ([fe3fde8](fe3fde8)) * **agents:** the Playwright specs have been in CI since 2026-08-08 ([#768](#768)) ([68ee612](68ee612)) * **aspire:** record the second local database and pin the AppHost dashboard ports ([#623](#623)) ([713b941](713b941)) * compress AGENTS.md to one paragraph per rule, and draw the two orders that matter ([#551](#551)) ([997ae8a](997ae8a)) * item 7 names each screen's actual initial filter value ([#666](#666)) ([70a53d8](70a53d8)) * multi-farm tenancy decision record and AGENTS/GLOSSARY sync ([#537](#537)) ([#601](#601)) ([2c34771](2c34771)) * name the scoped filtered-empty key and state the [#653](#653) relationship plainly ([#666](#666)) ([0e93dac](0e93dac)) * note that a PackageReference in Directory.Build.props is invisible to the dependency graph ([4845724](4845724)) * **plans:** commit the [#722](#722) and [#745](#745) design records ([#754](#754)) ([c942fcd](c942fcd)) * record [#579](#579) as won't-fix — suspension is immediate for use, not issuance ([#582](#582)) ([7a3be40](7a3be40)) * record the [#508](#508) audit ordering key and the tracked-file guard lesson ([#701](#701)) ([08964e9](08964e9)) * **runbooks:** add procedure to rename the default farm's code after upgrade ([#731](#731)) ([2f6e242](2f6e242)) * screenshots of the running SPA in the README ([#550](#550)) ([711488a](711488a)) * **sim:** commit the dashboard screenshot, capture the palette matrix, and record the [#651](https://github.com/mforce/cluckwork/issues/651)/[#652](https://github.com/mforce/cluckwork/issues/652) conventions ([#660](#660), [#662](#662), [#663](#663), [#664](#664)) ([#665](#665)) ([930ea30](930ea30)) * specify searchable entity picker ([#641](#641)) ([91d4300](91d4300)) * split the README into audience-scoped docs and adopt repo-template scaffolding ([#548](#548)) ([b3f3fcf](b3f3fcf)) * surface Aspire local development workflow ([#568](#568)) ([a343baa](a343baa)) * **web:** record the per-screen idempotency-key policies and runWrite's refresh contract ([#703](#703)) ([#707](#707)) ([8bee651](8bee651)) * **web:** the date-cap help text covers every stocked item, not only feed ([#666](#666), [#667](#667)) ([c8433c5](c8433c5)) * **web:** the help text claims only what is true of recording, and says nothing about filter caps ([#666](#666), [#667](#667)) ([e2f63d1](e2f63d1)) * **web:** the help text describes the date-range filters that shipped ([#666](#666), [#667](#667)) ([c3275b7](c3275b7)) * **web:** the help text stops describing a cap the filters no longer have ([#666](#666), [#667](#667)) ([49654cd](49654cd)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: cluckwork-lockfix[bot] <309265648+cluckwork-lockfix[bot]@users.noreply.github.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.
Two rule changes to
AGENTS.md, both owner directives, 2026-09-11.1. Removes the commit/push gate
Drops
AGENTS.md:210, "Only commit/push when the human asks".Agents may now commit, push a branch and open a PR without asking. The merge stays the owner's, and
mainis protected, so nothing reachesmainunreviewed either way.Why it goes rather than gets narrowed. The rule carried no
→decision-record link, so by this file's own preamble it was "a plain convention that has not yet cost anything" rather than an earned rule — no archaeology to follow before changing it. It was also ambiguous about where the gate actually sat: an agent working in an isolated worktree has to decide whether every commit waits or only the push, and the two readings differ a lot in practice. That ambiguity cost a round of clarification mid-way through #721.What it changes for an agent. Everything up to and including opening the PR becomes autonomous. Worth being explicit about the consequence, since it is the one thing the rule was implicitly buying: a PR opened without asking starts CI and spends a round of the review bot's included budget. That is the trade being accepted, not an oversight.
No other file restates the gate — checked
CONTRIBUTING.md,docs/and.github/.2. Broadens the screenshot rule
The #662 rule already required a 1:1 before/after comparison, but only for a PR whose purpose is visual. That trigger is too narrow: a PR whose purpose is a domain rule can still add a dialog, a field or a label, and those are exactly the changes a reader cannot check from the diff.
It now reads: any PR that changes what a user sees attaches screenshots, before and after wherever a before exists, same viewport and same scenario in both, after-only where the UI is net-new.
Two additions to the mechanics beside the existing ones (capture at 1:1, rebuild the stack at the head under review):
gh pr comment --attach, never by committing images to a branch. A branch is deletable, so routine cleanup silently breaks the images on a merged PR while the prose around them still claims they exist.This PR changes no UI, so rule 2 does not apply to it.
Related
#756 is the slice that surfaced both: its own ambiguity about the commit gate, and a confirm dialog that the old screenshot trigger would not have required.