diff --git a/.greptile/config.json b/.greptile/config.json new file mode 100644 index 0000000..cf894dd --- /dev/null +++ b/.greptile/config.json @@ -0,0 +1,139 @@ +{ + "strictness": 1, + "triggerOnUpdates": true, + "triggerOnDrafts": true, + "statusCheck": true, + "ignorePatterns": "node_modules/**\ndist/**\ncoverage/**\n.git/**\npnpm-lock.yaml", + "summarySection": { + "included": true, + "collapsible": true, + "defaultOpen": false + }, + "issuesTableSection": { + "included": true, + "collapsible": false + }, + "confidenceScoreSection": { + "included": true, + "collapsible": true, + "defaultOpen": false + }, + "instructions": "This repository is the neutral workspace.json PRODUCER, not the standard. It owns deterministic generation, repository scanning, manual-evidence preservation, atomic writes, drift detection, the explicit opt-in commit-history pass (L0/L1 mining), and the frozen `agents-audit` compatibility bridge. It CONSUMES `@workspacejson/spec` and `@workspacejson/rules` from `workspacejson/standard` at exact published versions and never redefines them. The dominant defect class here is a producer that emits a plausible-looking artifact for evidence it did not gather: a refused history refresh reported as a successful one, an empty coChange block written for a repository that could not be analyzed, a basisRevision advanced without recounting, or a preserved block silently rebuilt. Absence, refusal, shallow clone, and Git failure are all reportable outcomes and none of them is zero, empty, or green. Review changes for whether the artifact would state a result nobody measured. See .greptile/rules.md for the full prose and OWNERSHIP.md for the authority boundary.", + "rules": [ + { + "id": "evidence-load-bearing", + "rule": "Evidence claims must be load-bearing. A verification check that cannot fail proves nothing. If a test, guard, parity harness, or assertion is added or changed, it must be capable of failing for the defect it names. A harness that reports a pass count it did not compute, or that counts a skipped case as a passing one, is documentation rather than verification.", + "severity": "high" + }, + { + "id": "absence-not-success", + "rule": "Absence, refusal, skipped, unsupported, or unavailable is never success, false, safe, empty, zero, or green. This is the primary producer failure class in this repository: a shallow clone, an absent commit graph, or a failed Git invocation must produce a reported refusal, never a result. Converting a refusal into an empty collection, a zero count, or a silently omitted field states a measurement nobody took.", + "severity": "high" + }, + { + "id": "metrics-perturb", + "rule": "Metrics and receipts must perturb when their referent changes. A count, score, hash, or parity number that does not move when its input moves is decorative. If a metric or receipt is added or updated, verify it responds to a change in the underlying value it claims to measure.", + "severity": "high" + }, + { + "id": "bidirectional-checks", + "rule": "Every verification check must be considered in both directions: cannot-ever-pass and cannot-ever-fail. A guard that rejects everything and a working guard look identical from a green build. When a check, guard, or conformance step is added or modified, state what it would look like if it were broken in each direction.", + "severity": "high" + }, + { + "id": "clean-room-boundary", + "rule": "No `@marcelle-labs/*` scope, private Vreko source, `workspace.vreko.json`, or other cross-organizational implementation dependency may appear in any file, including config, comments, tests, and documentation. Additionally `packages/cli/` must contain no vendor- or host-specific content at all — no DataHub, dbt, Vreko, MCP, Codex, or editor-integration logic, by filename or by content. The neutral producer stays neutral.", + "severity": "high" + }, + { + "id": "history-refresh-refusal-observable", + "rule": "An explicitly requested history refresh must remain observable to the caller. When a caller passes `mineHistory: true` and mining refuses, the producer falls back to the previously preserved block — which is correct — but the result must still report that the refresh did not happen and why. Removing, narrowing, or conditionally omitting `historyRefresh` / `refusal`, or reporting `mined: true` on a fallback path, reintroduces the P1 found on PR #20: a successful-looking generation carrying the previous revision's counts that the caller cannot distinguish from a completed refresh.", + "scope": ["packages/cli/src/producer/**", "packages/cli/src/commands/**"], + "severity": "high" + }, + { + "id": "ordinary-generation-never-mines", + "rule": "Ordinary generation must not read the commit graph. History mining is explicit and opt-in: `mineHistory` defaults to off and that default is the contract, not a convenience. Any change that makes mining run by default, that inverts the flag's sense, or that adds a second commit-graph read path outside the mining pass, makes every ordinary run pay the mining cost and makes the artifact churn on every commit — the exact `generate --check` failure the raw-count amendment removed.", + "scope": ["packages/cli/src/producer/**", "packages/cli/src/commands/**", "packages/mining-core/src/**"], + "severity": "high" + }, + { + "id": "basis-pin-never-advances-without-recount", + "rule": "`generated.basisRevision` must not advance without recomputation. Carrying preserved observations forward while moving the pin to current HEAD re-attributes old counts to a commit they were never counted at; the numbers stay plausible and become false. `generatedAt` is not evidence about the history block and must never be used as its freshness signal. Only a completed mining pass may write a new basisRevision, and it must write the revision it actually counted at.", + "scope": ["packages/cli/src/producer/**", "packages/mining-core/src/**"], + "severity": "high" + }, + { + "id": "incomplete-history-emits-nothing", + "rule": "Shallow, absent, or unavailable history must never produce an apparently complete evidence block. A pinned empty `coChange` array is a positive finding — the analysis ran at this revision and found no qualifying pairs — so emitting one for a repository that could not be analyzed asserts a result that was never measured. A projection whose completeness is not a mined state must yield nothing at all, not an empty array, and the refusal must name which state it refused on.", + "scope": ["packages/mining-core/src/**", "packages/cli/src/producer/**"], + "severity": "high" + }, + { + "id": "canonical-utf8-endpoint-order", + "rule": "Observation pairs emitted by the producer use canonical UTF-8 byte order for their endpoints, established at the projection boundary. Bare `<` / `>` string comparison is UTF-16 code unit order and disagrees with UTF-8 above the BMP; `localeCompare` varies with host locale. Neither may be substituted for the UTF-8 comparator on any artifact-bound ordering, and endpoint reversal must not change the emitted bytes. Ordering claims require a test that fails when the ordering is violated, not prose.", + "scope": ["packages/mining-core/src/**", "packages/cli/src/producer/**"], + "severity": "high" + }, + { + "id": "preserved-history-survives-unchanged", + "rule": "Preserved commit-history evidence must survive unrelated ordinary generation byte-for-byte. `generated.coChange` and `generated.basisRevision` are the one part of the producer-owned section that ordinary generation preserves rather than rebuilds, because they derive from an input ordinary generation does not read. Passing the parsed values through untouched is what makes the guarantee real; rebuilding entries field by field re-orders keys and changes bytes even when the values match. Dropping the block on regeneration is indistinguishable, in the resulting artifact, from a repository that was never mined.", + "scope": ["packages/cli/src/producer/**"], + "severity": "high" + }, + { + "id": "validator-not-bypassed", + "rule": "`WorkspaceJsonValidator` from `@workspacejson/rules` may not be bypassed, replaced with a vendored or hand-rolled schema, stubbed in a non-test path, or weakened to accommodate candidate development. If a published type lags the amended schema, narrow the compile-time gap at a single declared boundary and say so — the runtime artifact still goes through the real validator unmodified. A local interface that describes a shape the validator has not accepted is a compile-time accommodation, never a relaxation of what the producer may emit.", + "scope": ["packages/cli/src/**", "packages/agents-audit-compat/src/**"], + "severity": "high" + }, + { + "id": "exact-standard-dependency-boundary", + "rule": "`@workspacejson/spec` and `@workspacejson/rules` are consumed at exact published versions. No committed `file:`, `link:`, `portal:`, `workspace:`, pnpm `overrides`/`resolutions`, or sibling-checkout path may substitute for the registry package, and no range specifier (`^`, `~`, `*`, `latest`) may replace an exact pin. A sibling override makes the build depend on an unpublished working tree, so CI green proves nothing about what a consumer installing from the registry receives.", + "scope": ["package.json", "packages/*/package.json", "pnpm-workspace.yaml", ".npmrc"], + "severity": "high" + }, + { + "id": "candidate-substitution-detected", + "rule": "Candidate-contract verification must detect registry substitution and stale install/cache substitution. A verifier that installs a package by name and checks that it works can pass against the previously published version, a cached tarball, or a registry-resolved package that is not the candidate under test. Verification must bind to the specific artifact — integrity/version/resolved-source — and must fail when what it received is not what it was asked to verify. Reusing a cache without proving the cache holds the candidate is the same defect wearing a different hat.", + "scope": ["scripts/**", "migration/**", ".github/workflows/**"], + "severity": "high" + }, + { + "id": "frozen-compat-bridge", + "rule": "`agents-audit` is frozen at 0.4.4 and is a compatibility bridge, not a development surface. No feature may be added to it, no workflow may publish it, and it must stay under `ignore` in the Changesets config so a workspace-wide version bump cannot move it. Anything touching its command surface, exit codes, output, or exports must be measured against the frozen pre-migration source via the parity harnesses, and a new parity difference requires an explicit recorded intentional-difference entry rather than an updated expectation.", + "scope": ["packages/agents-audit-compat/**", "migration/**", ".changeset/config.json", ".github/workflows/**"], + "severity": "high" + }, + { + "id": "producer-emits-no-derived-or-prescriptive-fields", + "rule": "The emitted artifact stays descriptive and count-based. The producer must not write derived probability fields (rate, confidence, lift, ranking) into observation-form output — observations are raw integer counts and a reader derives ratios — and must not introduce enforcement, approval-gate, or merge-blocking fields into any emitted section. A continuous derived value moves on every commit and makes `generate --check` fire forever; a prescriptive field makes the producer mandate what consumers must do with the artifact, which is not this repository's authority.", + "scope": ["packages/cli/src/producer/**", "packages/mining-core/src/**"], + "severity": "high" + }, + { + "id": "reader-tolerance-does-not-relax-producer", + "rule": "Reader tolerance does not relax producer obligations. That a reader, a published type, or the validator accepts a missing or malformed field does not license the producer to emit it that way. Do not widen what the producer writes because a consumer happens to tolerate it, and do not treat a permissive published type as permission to emit a shape the schema does not describe.", + "scope": ["packages/cli/src/**", "packages/agents-audit-compat/src/**"], + "severity": "high" + }, + { + "id": "artifact-bytes-deterministic", + "rule": "Artifact-bound output must be a function of the repository state alone — no wall clock, no host locale, no environment, no absolute host paths, no float where an integer is required. Two runs at the same basis must produce byte-identical bytes. `localeCompare` is prohibited in any serialization or ordering path that reaches the artifact because it varies with host locale. A non-finite or non-integer number in artifact-bound output must throw rather than round or drop: rounding invents precision the measurement does not have, and dropping removes a field a reader was told to expect.", + "scope": ["packages/mining-core/src/**", "packages/cli/src/producer/**"], + "severity": "high" + }, + { + "id": "artifact-independently-useful", + "rule": "The committed artifact must remain daemon-free and independently useful with nothing running. No emitted field, example, or documentation may make the artifact's meaning depend on a live service, daemon, running process, or network call. A consumer who clones a repository and reads `.agents/workspace.json` must be able to understand what it says without starting anything.", + "scope": ["packages/cli/src/producer/**", "README.md", "packages/cli/README.md"], + "severity": "medium" + }, + { + "id": "no-local-schema-redefinition", + "rule": "This repository does not define the specification. The normative schema, validation semantics, field lifecycle, and the stable read paths belong to `workspacejson/standard` and are consumed here as released packages. A local copy, fork, patch, or ambient-type shadow of a standard-owned contract is a boundary violation even when it typechecks — the shadowed `node:fs` ambient declaration already produced one such defect. Widening producer behavior to match a locally edited contract is not a fix.", + "scope": ["packages/cli/src/**", "packages/mining-core/src/**", "types/**", "packages/*/tsconfig.json"], + "severity": "high" + } + ] +} diff --git a/.greptile/files.json b/.greptile/files.json new file mode 100644 index 0000000..ffc56d2 --- /dev/null +++ b/.greptile/files.json @@ -0,0 +1,44 @@ +{ + "files": [ + { + "path": "OWNERSHIP.md", + "description": "The authority boundary for this repository: what the CLI owns (neutral producer, command routing, the frozen agents-audit compatibility surface, CLI distribution), what it consumes (released @workspacejson/spec and @workspacejson/rules at exact versions), and what it must never define (normative schema, host integrations, vendor consumer logic, private product behavior). Also records the clean-room boundary, the publish authority table, and the META-248 datahub-adapter extraction. Machine-checked by scripts/check-architecture.mjs." + }, + { + "path": "REVIEW.md", + "description": "The repo-owned review and merge policy. Defines what the Greptile Review check does and does not mean, why check completion is not semantic approval, how actionable findings are reconciled individually, the current-head rule, and the calibrated role split between Greptile and Sourcery." + }, + { + "path": "packages/cli/src/producer/generate.ts", + "description": "The producer entry point. Assembles the artifact, decides between mined and carried-forward history by exactly one of two routes and never both, reports the history-refresh outcome so a refused refresh cannot read as a completed one, refuses to overwrite an invalid artifact, and writes atomically. The material projection excludes generatedAt and by so drift detection does not fire on every run." + }, + { + "path": "packages/cli/src/producer/history-carry-forward.ts", + "description": "Why generated.coChange and generated.basisRevision are preserved rather than rebuilt by ordinary generation, and the three failure modes it is written against: dropping mined evidence, advancing the pin without recounting, and recomputing on every run. Validates rather than trusts the prior block, and passes parsed values through untouched so the byte-for-byte guarantee is real." + }, + { + "path": "packages/cli/src/producer/history-mine.ts", + "description": "The only code path that reads the commit graph, and it runs only when a caller explicitly asks. A refusal returns undefined, which is not the same as an empty result: a shallow clone, absent history, or a Git failure produce nothing here and the reason is surfaced on the diagnostics rather than discarded." + }, + { + "path": "packages/mining-core/src/project.ts", + "description": "The L1 projection boundary — the only place a selection becomes artifact-shaped. Establishes canonical UTF-8 endpoint order here rather than upstream, stores no derived value, omits the classification flag by design, and refuses rather than degrades when completeness is not a mined state." + }, + { + "path": "packages/mining-core/src/select.ts", + "description": "The selection rule and the UTF-8 byte comparator. Documents why bare string comparison (UTF-16 code units) and localeCompare are both wrong for artifact-bound ordering, and why the ranking keys on integer counts so no float decides an order." + }, + { + "path": "packages/mining-core/src/serialize.ts", + "description": "Deterministic serialization and the float prohibition. Two runs at the same basisRevision produce byte-identical output; non-finite and non-integer numbers throw rather than round or drop. Records the exact scope of the determinism claim, including the known generated.hygiene exception it does not inherit." + }, + { + "path": "packages/mining-core/src/paths.ts", + "description": "The single path-normalization function for L0 and the record of which META-278 path-identity questions are ratified versus assumed. Assumptions are carried in the output so a reader sees a guess as a guess rather than as settled behavior." + }, + { + "path": "CONTRIBUTING.md", + "description": "Build and test commands, the parity harnesses that measure the frozen agents-audit bridge against its pre-migration source, and the expected parity results. A third runtime difference means something changed that should not have, or owes an explicit intentional-difference record." + } + ] +} diff --git a/.greptile/rules.md b/.greptile/rules.md new file mode 100644 index 0000000..4fbe8bc --- /dev/null +++ b/.greptile/rules.md @@ -0,0 +1,335 @@ +# Review rules for `workspacejson/cli` + +This repository is the neutral `workspace.json` **producer**. It owns +deterministic generation, repository scanning, manual-evidence preservation, +atomic writes, drift detection, the explicit opt-in commit-history pass, and the +frozen `agents-audit` compatibility bridge. It consumes `@workspacejson/spec` +and `@workspacejson/rules` from `workspacejson/standard` at exact published +versions. See [`OWNERSHIP.md`](../OWNERSHIP.md) for the authority boundary and +[`REVIEW.md`](../REVIEW.md) for how these rules are used at merge time. + +The rules below are enforced as structured rules in `config.json` and elaborated +here in prose so the reasoning is reviewable alongside the code. + +## The failure class this repository actually has + +Nearly every rule here is a specialization of one defect: **a producer that +emits a plausible-looking artifact for evidence it did not gather.** + +The artifact is the product. Nothing downstream can tell, from the bytes alone, +whether a `coChange` block was counted at the revision it is pinned to, whether +a refresh the caller asked for actually ran, or whether an empty array means +"analyzed, nothing found" or "never analyzed". Every one of those confusions +produces a file that validates, reads plausibly, and is false. + +Greptile found exactly this on PR #20, and it is the reason this repository has +its own policy rather than a copy of the standard's: an explicit +`mineHistory: true` request could fall back to stale preserved history and +return a successful-looking result, with the refusal reason computed and then +discarded. The artifact looked identical either way. + +When reviewing a change here, the question that catches the most is: **would +this artifact state a result nobody measured?** + +## Ecosystem-wide rules + +These five are shared across the `workspacejson` ecosystem and apply to every +file in this repository. They are carried because they are genuinely +repository-independent, not because the standard has them. + +### Evidence must be load-bearing + +A verification check that cannot fail proves nothing. If you add a test, guard, +parity harness, or assertion, it must be capable of failing for the defect it +names. + +Example violation: a parity harness that reports `27/29` from a recorded +expectation rather than from the comparison it just ran. The number looks like a +measurement and is a constant. + +### Absence is not success + +Absence, refusal, skipped, unsupported, or unavailable is never success, false, +safe, empty, zero, or green. + +This is the load-bearing rule of this repository, because the producer has four +distinct ways to gather nothing — shallow clone, absent history, Git invocation +failure, and genuinely no qualifying pairs — and only the last one is a result. + +Example violation: `mineHistoryBlock` catching a Git failure and returning an +empty array instead of `undefined`. The caller writes a pinned empty `coChange`, +which under A-009 positively asserts that the analysis ran and found nothing. + +### Measurements must perturb + +Metrics and receipts must perturb when their referent changes. A count, score, +hash, or parity number that does not move when its input moves is decorative. + +Example violation: a tarball receipt whose integrity hash is read from a +manifest rather than computed from the pack that was just produced. + +### Checks cannot be vacuous + +Every verification check must be considered in both directions: cannot-ever-pass +and cannot-ever-fail. A guard that rejects everything looks identical to a +working guard from a green build. + +Example violation: an architecture guard whose path filter no longer matches any +file. It reports zero violations forever, which is exactly what it reports when +the repository is clean. + +### Clean-room boundary + +No `@marcelle-labs/*` scope, private Vreko source, `workspace.vreko.json`, or +other cross-organizational implementation dependency may appear in any file, +including config, comments, tests, and documentation. + +This repository carries a stricter form as well: `packages/cli/` must contain +**no** vendor- or host-specific content at all — no DataHub, dbt, Vreko, MCP, +Codex, or editor-integration logic, by filename or by content. The DataHub +adapter was extracted under META-248 precisely because a consumer adapter is not +neutral producer architecture, and re-adding it is a machine-checked failure. + +## CLI-specific rules + +These are written from verified producer failure classes in this repository, not +adapted from another repository's surface. + +### An explicitly requested history refresh must remain observable + +Scope: `packages/cli/src/producer/**`, `packages/cli/src/commands/**` + +When a caller passes `mineHistory: true` and mining refuses, falling back to the +previously preserved block is **correct** — destroying evidence over a shallow +clone or a transient Git failure would be worse. Falling back *quietly* is the +defect. + +The result must report that the refresh did not happen and why. Removing +`historyRefresh`, narrowing it, conditionally omitting `refusal`, or reporting +`mined: true` on a fallback path all reintroduce the PR #20 P1. `basisRevision` +only helps a reader who already suspects something is wrong; a caller who asked +for fresh observations must be told without having to suspect. + +Note the shape of the field: `historyRefresh` is absent when no refresh was +requested, and present with `mined: false` plus a `refusal` when one was +requested and refused. Those two absences mean different things and must not be +collapsed. + +### Ordinary generation must not mine commit history + +Scope: `packages/cli/src/producer/**`, `packages/cli/src/commands/**`, +`packages/mining-core/src/**` + +Mining is explicit and opt-in. `mineHistory` defaults to off, and that default is +the contract rather than a convenience: mining a bounded window costs seconds to +tens of seconds, and a producer that recomputed history every run would make the +artifact churn on every commit — the exact `generate --check` failure the +raw-count amendment removed. + +Watch for the inverted default (`options.mineHistory !== false`), a caller that +passes the flag unconditionally, and any second commit-graph read path added +outside the mining pass. + +### `basisRevision` must not advance without recomputation + +Scope: `packages/cli/src/producer/**`, `packages/mining-core/src/**` + +Carrying preserved observations forward while moving the pin to current `HEAD` +re-attributes old counts to a commit they were never counted at. The numbers stay +plausible and become false. Only a completed mining pass may write a new +`basisRevision`, and it must write the revision it actually counted at. + +`generatedAt` is not evidence about the history block — it records when the +ordinary run happened. A run that refreshes `fileIndex` moves `generatedAt` and +must leave `basisRevision` exactly where it was. A reader comparing `generatedAt` +to the current revision learns nothing about whether the history block is stale. + +### Shallow or unavailable history must emit nothing, not an empty block + +Scope: `packages/mining-core/src/**`, `packages/cli/src/producer/**` + +A *pinned* empty `coChange` array is a positive finding: the analysis ran at this +revision and found no qualifying pairs. Emitting one for a repository that could +not be analyzed states a result nobody measured. A projection whose completeness +is not a mined state must yield nothing at all, and the refusal must name the +state it refused on. + +The two mined states (`QUALIFYING_RELATIONSHIP_OBSERVED`, +`MINED_NO_QUALIFYING_RELATIONSHIP`) are the only ones that may produce a block. +Widening that set, or defaulting an unknown completeness state into it, is the +violation. + +### Observation pairs use canonical UTF-8 endpoint ordering + +Scope: `packages/mining-core/src/**`, `packages/cli/src/producer/**` + +Canonical endpoint order is established at the projection boundary, using the +UTF-8 byte comparator. Bare `<` / `>` string comparison is UTF-16 code unit +order and disagrees with UTF-8 above the BMP — a path containing U+1F600 sorts +before one containing U+E000 under UTF-16 and after it under UTF-8. +`localeCompare` varies with host locale. + +Upstream stages may sort with `<` for map keying, where only stability matters. +That is not fine for bytes a second producer is compared against. Endpoint +reversal must not change the emitted output, and that must be asserted by a test +rather than in prose. + +### Preserved history must survive unrelated generation byte-for-byte + +Scope: `packages/cli/src/producer/**` + +`generated.coChange` and `generated.basisRevision` are the one part of the +producer-owned section that ordinary generation preserves rather than rebuilds, +because they derive from an input ordinary generation does not read. + +Passing the parsed values through untouched is what makes the guarantee real. +Rebuilding entries field by field re-orders keys and changes bytes even when +every value matches. Dropping the block on regeneration produces an artifact +indistinguishable from one that was never mined. + +Validating rather than trusting the prior block is deliberate and must stay: a +block that would not survive schema validation must not be carried into a fresh +artifact, or one bad mining run poisons every subsequent generation. + +### `WorkspaceJsonValidator` may not be bypassed + +Scope: `packages/cli/src/**`, `packages/agents-audit-compat/src/**` + +The validator may not be bypassed, replaced with a vendored or hand-rolled +schema, stubbed in a non-test path, or weakened to accommodate candidate +development. + +There is a legitimate pattern that looks similar and is not a violation: when a +published type lags the amended schema, a local interface may narrow the +compile-time gap at a single declared boundary, provided the runtime artifact +still goes through the real validator unmodified and the accommodation says so. +The test is whether the local declaration changes what is *emitted* or only what +*compiles*. Changing what is emitted is the violation. + +### The standard dependency boundary is exact and registry-bound + +Scope: `package.json`, `packages/*/package.json`, `pnpm-workspace.yaml`, +`.npmrc` + +`@workspacejson/spec` and `@workspacejson/rules` are consumed at exact published +versions. No committed `file:`, `link:`, `portal:`, `workspace:`, pnpm +`overrides`/`resolutions`, or sibling-checkout path may substitute for the +registry package, and no range specifier may replace an exact pin. + +A sibling override makes the build depend on an unpublished working tree, so CI +green proves nothing about what a consumer installing from the registry +receives. A local override used during development must not be committed. + +### Candidate verification must detect substitution + +Scope: `scripts/**`, `migration/**`, `.github/workflows/**` + +A verifier that installs a package by name and checks that it works can pass +against the previously published version, a cached tarball, or a +registry-resolved package that is not the candidate under test. Verification must +bind to the specific artifact — integrity, version, resolved source — and must +fail when what it received is not what it was asked to verify. + +Reusing a cache without proving the cache holds the candidate is the same defect. +So is a post-publish check that cannot distinguish "the new version installs" +from "some version installs". + +### `agents-audit` is frozen + +Scope: `packages/agents-audit-compat/**`, `migration/**`, +`.changeset/config.json`, `.github/workflows/**` + +`agents-audit` is frozen at `0.4.4` and is a compatibility bridge, not a +development surface. No feature may be added to it, no workflow may publish it, +and it must stay under `ignore` in the Changesets config so a workspace-wide +version bump cannot move it. + +Anything touching its command surface, exit codes, output, or exports must be +measured against the frozen pre-migration source via the parity harnesses. A new +parity difference requires an explicit recorded intentional-difference entry — +updating the expected count to match the new behavior is the violation, not the +fix. + +### The producer emits no derived or prescriptive fields + +Scope: `packages/cli/src/producer/**`, `packages/mining-core/src/**` + +Observation-form output carries raw integer counts. No `rate`, probability, +confidence, lift, or ranking may be written into it; a reader derives ratios. A +continuous derived value moves on every commit and makes `generate --check` fire +forever. + +The artifact also stays descriptive. The producer must not introduce enforcement, +approval-gate, or merge-blocking fields into any emitted section — mandating what +consumers must do with the artifact is not this repository's authority. + +Absence of the optional classification flag is a positive design decision, not an +unfinished one: this producer implements no deterministic tooling-coupling +classifier, so it says nothing rather than emitting a constant `false`. + +### Reader tolerance does not relax producer obligations + +Scope: `packages/cli/src/**`, `packages/agents-audit-compat/src/**` + +That a reader, a published type, or the validator accepts a missing or malformed +field does not license the producer to emit it that way. Do not widen what the +producer writes because a consumer tolerates it, and do not treat a permissive +published type as permission to emit a shape the schema does not describe. + +### Artifact bytes are deterministic + +Scope: `packages/mining-core/src/**`, `packages/cli/src/producer/**` + +Artifact-bound output must be a function of repository state alone — no wall +clock, no host locale, no environment, no absolute host paths. Two runs at the +same basis produce byte-identical bytes. + +`localeCompare` is prohibited in any serialization or ordering path that reaches +the artifact. A non-finite or non-integer number in artifact-bound output must +**throw** rather than round or drop: rounding invents precision the measurement +does not have, and dropping removes a field a reader was told to expect. Both are +quieter than the bug, and quiet is the failure mode this repository exists to +avoid. + +One known and accepted exception is recorded rather than hidden: +`generated.hygiene` in the published producer is fed by a 30-day +`git log --since` window and moves on its own (META-306). L0 does not inherit +that and does not fix it. Do not cite it as precedent for new nondeterminism. + +### The artifact stays independently useful + +Scope: `packages/cli/src/producer/**`, `README.md`, `packages/cli/README.md` + +The committed artifact must remain daemon-free. No emitted field, example, or +documentation may make its meaning depend on a live service, daemon, running +process, or network call. A consumer who clones a repository and reads +`.agents/workspace.json` must understand what it says without starting anything. + +### This repository does not define the specification + +Scope: `packages/cli/src/**`, `packages/mining-core/src/**`, `types/**`, +`packages/*/tsconfig.json` + +The normative schema, validation semantics, field lifecycle, and the stable read +paths belong to `workspacejson/standard`. A local copy, fork, patch, or +ambient-type shadow of a standard-owned contract is a boundary violation even +when it typechecks — the shadowed `node:fs` ambient declaration in +`types/ambient.d.ts` already produced one such defect, and the same class was +fixed once already for `@workspacejson/spec`. + +Widening producer behavior to match a locally edited contract is not a fix. If +the contract is wrong, that is an issue on `workspacejson/standard`. + +## Deliberately not carried from `workspacejson/standard` + +Recorded so the omissions read as decisions rather than oversights. + +| Standard rule | Why it is not here | +| -- | -- | +| `four-read-paths-breaking` | The read paths are a schema surface owned by `standard`. This repository's obligation is not to redefine them locally, which `no-local-schema-redefinition` already states. Duplicating the ADR-gating rule here would claim authority this repository does not hold. | +| `negative-fixtures-single-defect` | Scoped to `packages/spec/examples/invalid/**` and the standard's fixture machinery. No equivalent surface exists here. | +| `descriptive-not-prescriptive` (schema form) | Carried only in its producer form — what the producer may *emit* — inside `producer-emits-no-derived-or-prescriptive-fields`. The schema-authoring half does not apply. | +| `daemon-free` (spec/docs form) | Carried in its artifact form as `artifact-independently-useful`, scoped to producer output rather than to specification text. | +| `no-derived-probability` (spec form) | Carried as a producer emission obligation in the same combined rule, because this repository is where the emission actually happens. | +| `cochange-ordering-evidence` (spec form) | Carried as `canonical-utf8-endpoint-order`, which names the concrete comparator defect this repository can commit rather than the documentation claim `standard` can commit. | +| `reader-producer-distinct` (spec form) | Carried as `reader-tolerance-does-not-relax-producer`, stated from the producer's side. | diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b35488c..dc22714 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -77,6 +77,19 @@ or you owe it an explicit intentional-difference record. - `@workspacejson/cli` losing `private: true` - any workflow attempting to publish `@workspacejson/spec` or `@workspacejson/rules` +## Review + +[`REVIEW.md`](./REVIEW.md) is the review and merge contract: which checks are +required, why a completed review check is not the same as approval, and why +findings are reconciled one thread at a time. + +The semantic rules the automated reviewer applies are repo-owned and live in +[`.greptile/rules.md`](./.greptile/rules.md). They are written from producer +failure classes this repository has actually hit — a refused history refresh +reported as a successful one, a basis pin advanced without recounting, an empty +evidence block emitted for a repository that could not be analyzed. Read them +before touching `packages/cli/src/producer/**` or `packages/mining-core/src/**`. + ## Reporting Issues File bugs at [GitHub Issues](https://github.com/workspacejson/cli/issues). diff --git a/REVIEW.md b/REVIEW.md new file mode 100644 index 0000000..4e839ad --- /dev/null +++ b/REVIEW.md @@ -0,0 +1,113 @@ +# Review and merge policy — `workspacejson/cli` + +This is the repo-owned review contract. The semantic rules the automated +reviewer applies live in [`.greptile/rules.md`](./.greptile/rules.md) and +[`.greptile/config.json`](./.greptile/config.json); this document covers what +the checks mean, what they do not mean, and what has to be true before a change +merges. + +## What runs on a pull request + +| Check | App | Required for merge | +| -- | -- | -- | +| `test (20)`, `test (22)` | GitHub Actions | yes | +| `Compatibility parity vs frozen source` | GitHub Actions | yes | +| `Greptile Review` | Greptile | see [Status](#status-of-the-greptile-gate) | +| `Sourcery review` | Sourcery | **no**, by decision — see below | +| `Socket Security` | Socket | no | + +Branch protection on `main` additionally requires branches to be up to date +before merging (`strict`) and requires conversation resolution. + +## Check completion is not semantic approval + +A completed `Greptile Review` check means the review **ran to completion on that +commit**. It does not mean the change was approved, and a green check is not +evidence that a finding was addressed. + +The two are carried by different mechanisms, and conflating them is the failure +this section exists to prevent: + +* the **status check** answers "did review complete on this head?" +* **conversation resolution** answers "was every actionable finding dealt with?" + +A pull request with a green Greptile check and an unresolved actionable thread +has not satisfied review. Required conversation resolution is what makes that +enforceable rather than aspirational. + +## The current-head rule + +Review evidence is bound to the commit it was produced on. `triggerOnUpdates` is +enabled, so a new push re-runs review against the new head, and evidence from an +earlier head does not carry forward to it. + +This means: + +* a review completed on an earlier commit does not satisfy the gate for a later + one; +* `strict: true` prevents merging a branch that is behind `main`, so the head + that was reviewed is the head that merges; +* re-requesting review after a push is not optional politeness — the previous + result describes code that is no longer what would merge. + +Do not treat a check run you can see in the PR timeline as current without +confirming its `head_sha` matches the head you intend to merge. + +## Reconciling findings + +Every actionable finding is reconciled **individually**, on its own thread, with +either a fix or a stated reason it does not apply. + +Bulk resolution is not evidence. Resolving a batch of threads in one action +records that someone clicked resolve, not that each finding was considered, and +it is specifically not accepted as reconciliation for the rules in +`.greptile/rules.md`. + +A finding may be closed as not-applicable. Say why on the thread. "Not +applicable" without a reason is indistinguishable from "not read". + +## Greptile and Sourcery are different layers + +They are not redundant reviewers and are not treated as interchangeable. + +**Greptile** is the semantic layer. It reads the repo-owned rules in +`.greptile/`, which encode this repository's actual producer failure classes. It +found the PR #20 P1 — an explicit history refresh that could fall back to stale +preserved evidence without telling the caller — which is the concrete evidence +that CLI-specific semantic rules catch things generic review does not. + +**Sourcery** is a second-review and reconciliation layer, explicitly triggered. +It is **not** a required GitHub check, and it does not become one merely because +a `Sourcery review` check run exists on recent heads. Promoting it to a hard gate +requires its own calibration evidence, on the same terms Greptile was held to. +Until that exists, it remains defense-in-depth. + +## Status of the Greptile gate + +`Greptile Review` is **not yet a required branch-protection status** for `main`. + +The rollout is deliberately staged: a repo-owned policy lands first, a +disposable canary PR then proves the policy behaves correctly against real +CLI failure classes, and only then is the required-check decision made. + +A rule that misses its own positive control is not eligible to become a hard +gate. Making a check required before proving it can fail for the defect it names +would install a gate that reports conformance it never measured — which is the +same defect `.greptile/rules.md` exists to catch in the producer. + +Calibration and the resulting decision are tracked on META-321, and this section +is updated with the measured outcome rather than with an intention. + +## For agents and automated contributors + +* Read [`OWNERSHIP.md`](./OWNERSHIP.md) before changing anything: this + repository consumes the standard, it does not define it. +* Read [`.greptile/rules.md`](./.greptile/rules.md) before changing + `packages/cli/src/producer/**` or `packages/mining-core/src/**`. Most rules + there describe a defect that has actually shipped somewhere in this + ecosystem. +* Do not weaken a producer guard, a parity expectation, or a validator call to + make a change pass. Adjusting the measurement to match the behavior is the + violation, not the fix. +* Report `not run`, `unavailable`, `unsupported`, and `absent` distinctly from + pass and fail when recording evidence. Exit 0 alone is not a receipt. diff --git a/scripts/check-architecture.mjs b/scripts/check-architecture.mjs index 5acfb03..38f7dac 100644 --- a/scripts/check-architecture.mjs +++ b/scripts/check-architecture.mjs @@ -43,6 +43,19 @@ walk(repoRoot, (file) => { // This file legitimately names every forbidden pattern, as does the ownership // documentation that explains the rules. Excluding them by path keeps the guard // from flagging its own vocabulary while still scanning all real source. +// +// The review policy under .greptile/ joined this set for the same reason and +// not for a weaker one: the clean-room rule has to NAME the forbidden scopes to +// be enforceable, so the two files that state it necessarily trip the guard +// that enforces it. Paraphrasing the names out of the policy would have turned +// a green build into the goal and left the reviewer without the strings it +// matches on — adjusting the measurement to fit the behavior, which the policy +// itself prohibits. +// +// Membership is by exact path, never by directory prefix, so a new file under +// .greptile/ is still scanned. That is asserted by a red case in +// check-architecture.test.mjs rather than left to this comment: "two files are +// exempt" and "the directory is exempt" look identical from a passing run. const SELF_REFERENTIAL = new Set([ join(repoRoot, "scripts", "check-architecture.mjs"), join(repoRoot, "scripts", "check-architecture.test.mjs"), @@ -52,6 +65,8 @@ const SELF_REFERENTIAL = new Set([ join(repoRoot, "CHANGELOG.md"), join(repoRoot, "AGENTS.md"), join(repoRoot, "migration", "PROVENANCE.md"), + join(repoRoot, ".greptile", "config.json"), + join(repoRoot, ".greptile", "rules.md"), ]); // --------------------------------------------------------------------------- diff --git a/scripts/check-architecture.test.mjs b/scripts/check-architecture.test.mjs index b82aaff..0888e65 100644 --- a/scripts/check-architecture.test.mjs +++ b/scripts/check-architecture.test.mjs @@ -36,6 +36,19 @@ const cases = [ mutate: (root) => write(join(root, "packages/agents-audit-compat/src/violation.ts"), `export const sidecar = '.agents/workspace.vreko.json';\n`), }, + { + // The review policy names the forbidden scopes because the rule is + // unenforceable without them, so .greptile/config.json and + // .greptile/rules.md are in SELF_REFERENTIAL. This asserts the exemption is + // by exact path and did not become a directory-wide hole: any OTHER file + // under .greptile/ is still scanned. Without this case, "two files are + // exempt" and "the directory is exempt" are indistinguishable from a green + // run — the cannot-ever-fail direction of the same check. + name: "clean-room: .greptile exemption is per-file, not directory-wide", + expect: "clean-room", + mutate: (root) => write(join(root, ".greptile/notes.md"), + `Scratch notes that reference @marcelle-labs/private-core.\n`), + }, { name: "copied-schema: normative schema copied into the CLI repo", expect: "copied-schema",