Conversation
…merated, never blinds a gate (#9199)
No preview deploy
|
…eview verb, governed-root refusal at 21 (ADR 0401) Cherry-picked from the spike branch (19009cec + ac16ae67, never merged), resolved against main: - GOVERNED_FILTER re-seated 17 -> 21 (main took 17 SUPERSEDES_VERDICT, 18-20 also landed) - review seat subcommand kept beside review preview - design-guard corpus paths updated to packages/design/ (main's layout) - --emit-diff defaults false (main's flag DSL treats a bare boolean flag as required) - CRLF-tolerant diff parsing (autocrlf checkouts / Windows-authored diffs) - refusal union narrowed to governedRoots; guard trees documented + drift-loud by golden sync test Verified: vitest src/review 652/652 (42 files), tsc + effect-tsgo clean.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
…use-style the preview read (CI gates) - portability-guard: strip ADR/issue numbers, .decisions/ and apps/web literals from fabrika-shipped text (ceiling 1 per row) - the CLI refusal test now derives its governed root from the runtime .fabrika.jsonc instead of naming a repository path; guard now clean (205 refs, all under ceilings) - biome: organize imports, format, drop unused import, rename escape -> escapeRegex (shadowed global) - preview file read: raw try/catch -> Effect.match folded into the file's tagged-union idiom (success-channel-only verbs); PRECONDITION_UNKNOWN refusal text unchanged Verified: vitest src/review 652/652, tsc 0 errors, effect-tsgo clean, portability-guard clean.
…(ADR 0401) - review preview gains its parity subjects: a PR number (optional --sha/--repo) and --base/--tip, each proving diff completeness before the filter, exactly as review diff does. - reviewSubsystems config key: path globs whose matched files carry additive subsystem / subsystem-note constraint rows in review scope output, matched by the filter's own glob engine and layered onto the class rubrics per the review skill's additive-constraints section. - reviewFilterExclusions / reviewFilterUnexclude config keys: repos extend the exclusion set and remove defaults; a removal is enumerated (un-excluded rows, x-fabrika-unexcluded-path: headers) rather than silent, and an equal-pattern re-addition deterministically wins. With both keys absent every consumer's output is byte-identical. - ADR 0401: Consequences updated for the config surface; Records entry records the 2026-09-20 completion. The placement ruling (#9199 Q1) stays open — --filter-placement carries both placements until it lands.
… completeness claim Three defects found by Baris Eroglu's reviewer agent at cb0b190, all reproduced at that head and fixed failing-tests-first (nine new tests in the filter-spike suite, eight red before the fixes, all green after): - git-quoted C-style diff headers fell through the filter's section splitter, whose regex matched only bare-form headers — a quoted first section vanished, and a quoted section after an excluded one was glued to it and dropped unenumerated. diffSections now parses headers through diff.ts's exported headerPaths — the same grammar the completeness proof counts with — so quoted and bare forms are one population and section counts cannot drift from the census. - a pattern naming a governed tree literally (governed/*.ts) slipped past the probe-granular refusal while still carving the tree out of the review's content. refusalFor gains a literal-target arm over the governed-root probes; wildcard-led patterns stay out of scope by a stated granularity limit, bounded by the excluded-path enumeration and the consumer split. - the completeness claim (every subject proves it) was false for --diff-file, which has no range to prove a census against. Wording across the preview verb, its CLI help and the ADR's records is scoped to the PR and range subjects; --diff-file reads bytes as given. Gates: 862/862 across src/review + src/config/keys (52 files), tsc 31/31, biome clean, portability guard clean (1447 files, none above ceiling).
…excluded-path backstop A wildcard-led exclusion pattern (a .ts suffix glob behind a leading double-star) carried no literal run, so the literal-target arm never fired and the probe arm could not see it: a governed file was silently carved out of the served diff. The pattern-level arm becomes a segment-wise forced-literal-alignment check — a pattern is refused when it pins a governed root's full path by literal segments alone — and a runtime backstop refuses in preview, scope, and diff whenever a path the filter actually excluded lies under a governed root. Pattern-level refusal stays fence-safe (generic deep globs like the shipped defaults are not refused on hypothetical matches); the backstop is what makes the contract complete over the real diff.
|
heal-ci: ROUTED — PR #9476 @ 47cbe8a → author Scheduled stall sweep: this pull request classifies as Detection only — this run merged nothing, re-ran nothing and spawned nothing. What to do about the flag is a driver decision; Posted by the |
|
heal-ci: ROUTED — PR #9476 @ 47cbe8a → human Scheduled stall sweep: this pull request classifies as Detection only — this run merged nothing, re-ran nothing and spawned nothing. What to do about the flag is a driver decision; Posted by the |
|
governance: FAIL @ 47cbe8a content:0ba6d1721b21 — scope requirements and zero-content policy disagree with standing rules Reviewed head: 47cbe8a. Governance FAIL. G1 — Requirement derivation disagrees with the standing review contract. In packages/fabrika-cli/src/review/scope-verb.ts, partitionSource becomes split.kept for explicit --filter-placement=before, and that narrowed input feeds partitionWithUi and shipNamespacesOf. Ship and lane requirement readers still use raw paths. Excluding the only ordinary code file therefore removes review-code from scope while the merge gate still requires it. The merge-base review skill anchor NAMESPACE-SET-IS-THE-EMISSION-CHECKLIST requires these sets to agree. ADR 0401's own consumer-split and Consequences promises say every requirement reader keeps raw paths; this implementation contradicts that promise. This is a requirement mismatch, not evidence that ship can be bypassed. Filtering is OFF when the scope/diff flag is omitted. G2 — ADR 0092 requires a relevant zero scan to fail closed, with legitimate non-applicability stated explicitly. The before branch can return successful scope with no classes/namespaces after excluding a nonempty raw diff. The proposed ADR leaves that exact policy OPEN, and neither records an amendment to 0092 nor supplies a consistent explicit skip contract. The after branch keeps requirements but also needs a review instruction for a deliberately empty served diff. These policies can change by a recorded ruling; the current code and standing instructions cannot disagree. The parent relayed the user's new choice, “reviewer can still stay,” during this review; that authorizes repair, not approval of the unchanged head. The anchor scan compared 23 blocks across 12 anchored rules with no existing anchor change. The new subsystem anchor is additive. No raw guard reader was changed to filtered input, and runtime governed-path refusal exists. Thus no guard bypass is alleged. The contradiction is the scope/merge requirement contract and its zero-content case. Corpus sweep read the candidate's domain and shortlisted 0313, 0250, 0229, 0239, 0252, 0253, 0322, 0398; no additional contradiction found in their queue, hook, relay, release, evaluation, lane-view, or local-config rules. Live statuses were resolved. 0300 as amended by 0400 was also read; the ADR candidly distinguishes recommendation from placement ruling. No inferred choice is treated as authorization. Governance judges those rules, not missing issue criteria or editorial quality. Superseded verdict — 2026-09-20governance: FAIL @ 47cbe8a content:0ba6d1721b21 — scope requirements and zero-content policy disagree with standing rules Reviewed head: 47cbe8a. Governance FAIL. G1 — Requirement derivation disagrees with the standing review contract. In packages/fabrika-cli/src/review/scope-verb.ts, partitionSource becomes split.kept for explicit --filter-placement=before, and that narrowed input feeds partitionWithUi and shipNamespacesOf. Ship and lane requirement readers still use raw paths. Excluding the only ordinary code file therefore removes review-code from scope while the merge gate still requires it. The merge-base review skill anchor NAMESPACE-ROWS-ARE-THE-GATE-SET requires these sets to agree. ADR 0401's own consumer-split and Consequences promises say every requirement reader keeps raw paths; this implementation contradicts that promise. This is a requirement mismatch, not evidence that ship can be bypassed. Filtering is OFF when the scope/diff flag is omitted. G2 — ADR 0092 requires a relevant zero scan to fail closed, with legitimate non-applicability stated explicitly. The before branch can return successful scope with no classes/namespaces after excluding a nonempty raw diff. The proposed ADR leaves that exact policy OPEN, and neither records an amendment to 0092 nor supplies a consistent explicit skip contract. The after branch keeps requirements but also needs a review instruction for a deliberately empty served diff. These policies can change by a recorded ruling; the current code and standing instructions cannot disagree. The parent relayed the user's new choice, “reviewer can still stay,” during this review; that authorizes repair, not approval of the unchanged head. The anchor scan compared 23 blocks across 12 anchored rules with no existing anchor change. The new subsystem anchor is additive. No raw guard reader was changed to filtered input, and runtime governed-path refusal exists. Thus no guard bypass is alleged. The contradiction is the scope/merge requirement contract and its zero-content case. Corpus sweep read the candidate's domain and shortlisted 0313, 0250, 0229, 0239, 0252, 0253, 0322, 0398; no additional contradiction found in their queue, hook, relay, release, evaluation, lane-view, or local-config rules. Live statuses were resolved. 0300 as amended by 0400 was also read; the ADR candidly distinguishes recommendation from placement ruling. No inferred choice is treated as authorization. Governance judges those rules, not missing issue criteria or editorial quality. |
|
review-code: FAIL @ 47cbe8a content:0ba6d1721b21 — missing criteria and concrete filter defects Reviewed head: 47cbe8a. Review-code FAIL, round 1. Scope: 36 changed files; code 34, doc 1, skill 1. Full served diff read, 36/36 sections. No implementation issue relationship is returned by review scope. A mixed code/skill PR requires linked issue acceptance criteria; discussion 9199 is not that input. Criteria remain UNKNOWN, not invented from PR prose. The required Deviations section is absent. C1 — packages/fabrika-cli/src/review/filter-spike.ts:58 does not escape '?' before constructing a RegExp. The declared grammar makes only '*' and '**' special and everything else literal (lines 60–65). A literal pattern file?.ts instead matches file.ts or fil.ts and misses file?.ts; '?' alone produces an invalid regex. Both exclusions and subsystem matching use this function. Escape the character and test both literal matching and the non-throwing single-character case. C2 — previewOf returns namespaces using plain partition and review-${class}, while runScope uses partitionWithUi plus shipNamespacesOf. For the same unexcluded governed file, preview omits governance; for configured rendered files it omits review-ui. Its after-branch comment promises the namespace rows stand unchanged, and ADR 0401 says preview shares derivation. Either derive the full actual namespace set using the same config or explicitly narrow and rename this output so it cannot be mistaken for the required reviews. PR/range/diff-file subject parity alone does not fix this difference. C3 — Explicit before filtering narrows scope's requirement set while ship/lane still use raw paths. Filtering defaults OFF; this does not prove a merge bypass. It does produce requirements a reviewer cannot satisfy by following scope. See separate governance findings. The supplied new user choice to keep the reviewer is repair direction; this head still exposes both placements. Tests inspected include config decoding, filtering, quoted filenames, runtime governed-path refusal, preview subjects and subprocesses, scope output, and guard-export checks. These do not exercise C1 or preview UI/governance parity. No local test execution at this exact head is claimed. CI will be reported separately when read; no green run overrides the source findings. Deviations: absent — required repair. No acceptance criterion appended because no implementation issue is linked. Verdict-written: 2026-09-20T21:40:49Z |
|
review-doc: FAIL @ 47cbe8a content:0ba6d1721b21 — missing criteria and inaccurate decision claims Reviewed head: 47cbe8a. Review-doc FAIL, round 1. Read the full ADR 0401, within the complete 36-file diff. This is an explanation/decision record. Missing implementation issue linkage leaves acceptance criteria UNKNOWN; this mixed PR does not qualify for the issueless-doc-only exception. Required Deviations section absent. D1 — The opening and Consequences say all requirement-deriving consumers retain raw paths and every namespace rule is untouched. scope-verb.ts passes kept paths into namespace derivation under explicit before, contradicting that claim. Records acknowledges both runtime placements while the Decision still leaves placement OPEN. Record the supplied final choice and align code and claims; do not imply current ship bypass, since ship still derives raw requirements. D2 — Context says mechanism 3 remains a separate proposal and only the first two mechanisms belong to this ADR. Records says all three now land, and the diff implements reviewSubsystems. Update the current scope consistently; a later diary entry does not make the opening accurate. D3 — Exact benchmark counts and the 21/21 claim-ledger result cite untracked benchmarks/ocr-port files to land in a future epic child. Those inputs are not reviewable in this diff. The reproduced commands cover two historical examples, not the census, token profiles, or claim ledger. Evidence for those quantitative claims is UNKNOWN. Supply a stable reviewable source for the relevant aggregate evidence or clearly bound/remove the unsupported exact claims before relying on them as the decision's measured basis. The record does distinguish recommendation, endorsement, and the unresolved ruling rather than silently inventing authority. The user's subsequent choice that the reviewer can stay enables a repair; it does not change the reviewed bytes. Deviations: absent — required repair. D4 — Context calls the benchmark founder-adjudicated, whereas Provenance identifies Baris Eroglu as adjudicator. Correct the attribution. Historical descriptions of wildcard-led patterns as out of scope should be clearly historical, since the final runtime backstop now handles their actual governed-path matches. Verdict-written: 2026-09-20T21:41:08Z |
|
review-skill: FAIL @ 47cbe8a content:0ba6d1721b21 — missing criteria and incomplete reviewer instructions Reviewed head: 47cbe8a. Review-skill FAIL, round 1. Self-review fence honored: judged with review SKILL.md and code/doc/skill rubrics at merge base be72614, not the proposed rules. Full head skill read. Missing implementation issue means criteria UNKNOWN; required Deviations section absent. S1 — The new instruction says to apply each subsystem-note whose subsystem row covers files in the current class. Scope emits only subsystem name, count and constraint, not matched paths, pattern, or classes. On a mixed code/doc/skill PR those rows cannot identify which class owes the constraint. Include a deterministic class/path association or instruct an explicit supported read that provides it. S2 — The runtime introduces filtered diffs and a successful all-excluded scope, but the skill still requires the complete diff and exact agreement between scope and merge requirements. It defines neither the deliberate empty-content review path nor how a reviewer should judge omitted content. The new additive-only paragraph cannot resolve that gap. Keep review requirements as the user now chose, and define what evidence the reviewer reads and what a zero-served-content verdict must establish. Do not approve solely because no content was served. The new scope row types and filtering flags also have no entries in the unchanged review contract.md. Update the caller contract together with the instruction, including exclusions and the complete requirement set. Existing text remains load-bearing; no new weaker rule was used to judge this PR. Deviations: absent — required repair. No acceptance criterion appended without an issue. Verdict-written: 2026-09-20T21:41:18Z |
3 participants
What this lands
ADR 0401 — Review diff filtering is deterministic, enumerated, and never blinds a gate —
the OCR-port benchmark outcome (local benchmark, 2026-09-12/13) and the four sub-rulings
resolved on discussion 9199.
2026-09-20; no on-thread comment — the ADR is the citation of record) and asked for this draft
to be linked on discussion 9199 for review. The endorsement covers the direction as posted; filter
placement (Q1) remains an open ruling — fields 2–4 carry the RFC's recommendations into the
ADR unchanged.
verification reads"; the reproducible
git showcommands are inline in the ADR's Provenancesection so any maintainer can re-run them in under a minute.
benchmarks/ocr-port/claim-ledger.mjs, 21/21 PASS2026-09-13 against a fresh live fetch of the posted thread) is cited in the ADR's Records
section.
Implementation in this diff
The working implementation, cherry-picked from the spike branch (
19009cec+ac16ae67— nevermerged) and resolved against moved main:
review scope/review diffgain the exclusion set (--filter-placement=before|after,--exclude), enumeratedexcluded/excluded-pathrows, and machine-readable diff headers(
x-fabrika-filter: placement=… excluded=N served=M, onex-fabrika-excluded-path:line perexcluded path) emitted strictly after the completeness proof.
review previewverb: the filtered path extraction with zero LLM turns; optional--emit-diffprints the filtered diff with its headers.governedRoots, refusing at 21 (GOVERNED_FILTER— thespike's seat 17 was taken on main by
SUPERSEDES_VERDICT; 18–20 also landed). Guard triggertrees stay documented and drift-loud by the golden sync test, not protected by refusal.
review seatsubcommand kept alongsidepreview; design-guard corpus paths updated topackages/design/(main's layout);--emit-diffdefaults false (main's flag DSL); CRLF-tolerant diff parsing (Windows checkouts /Windows-authored diffs).
Verification: review + config-key suites 879/879 green (52 files,
vitest run src/review/ src/config/keys/at47cbe8a0),tsc+ effect-tsgo clean, portability guard clean (1,447 files scanned, none above ceiling),.fabrika.schema.jsondrift gate green. Dual placement stays behind the flag deliberately — field 1 (Q1) is the open ruling; the shipped default collapses to whichever placement the ruling picks.Landed 2026-09-20 (
cb0b1909; review rounds7310dd6f,47cbe8a0) — all three mechanisms agreed on discussion 9199review previewgained its PR subject (a pull-request number withoptional
--sha/--repo) and range subject (--base/--tip). The PR and range subjects provediff completeness before the filter runs, exactly as
review diffdoes — a short read neverreaches the exclusion set;
--diff-filereads its bytes exactly as given (there is no range toprove a census against — its exclusions are still enumerated).
reviewSubsystemsconfig key —path globs whose matched files carry additive
subsystem/subsystem-noteconstraint rows inreview scopeoutput, layered onto the class rubrics per the review skill'sadditive-constraints section, matched by the filter's own glob engine so a repo never learns two
dialects.
reviewFilterExclusions(extend) andreviewFilterUnexclude(remove shipped defaults) in.fabrika.jsonc. A removal is enumerated,never silent —
un-excludedrows in scope output,x-fabrika-unexcluded-path:headers on thefiltered diff — and an equal-pattern re-addition deterministically wins over a removal. With both
keys absent, every consumer's output is byte-identical.
7310dd6f): three defects found by an external reviewer atcb0b1909,reproduced at that head and fixed failing-tests-first (nine new tests, eight red before, all
green after): git-quoted C-style diff headers (
diff --git "a/naïve.md" …) now parsethrough the same grammar the completeness proof counts with (a quoted section could previously
vanish or be glued-and-dropped); a pattern naming a governed tree literally (
governed/*.ts)refuses at 21; and the completeness claim above is scoped to the PR and range subjects.
47cbe8a0): the same reviewer's follow-up found the wildcard-ledgap — a
.tssuffix glob behind a leading double-star carries no literal run, so neither armrefused it and a governed file could be silently excluded (reproduced at
7310dd6f). Fixed intwo halves: the literal-target arm is replaced by a segment-wise forced-literal-alignment
check (a pattern refuses when it pins a governed root's path by literal segments alone — it
catches
**/governed/**-shaped pins the prefix run missed and never over-refuses the shippeddefaults), and a runtime backstop refuses in preview, scope, and diff whenever a path the
filter actually excluded lies under a governed root, which closes every generic pattern over
the real diff. Seventeen new tests; the module-level ones verified red at
7310dd6fbeforethe fix landed.
What this PR does NOT do
All three mechanisms agreed on discussion 9199 are landed above. These are the remaining scope boundaries, not
missing features:
benchmarks/ocr-port/untilthe epic's first child lands them under
reports/.spike/ocr-port-filter-placement(19009cec,ac16ae67) is reference material only; the implementation above was cherry-picked from it andresolved against main.
--filter-placementcarries bothbeforeandafteruntil Usirin rules on discussion 9199 Q1; nothing in this PR picks a default.