diff --git a/AGENTS.md b/AGENTS.md index 3a602828..e3da1560 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -21,7 +21,7 @@ A state-changing GitHub call is the highest-blast-radius thing an agent does her - **Write only to the current project's own repository.** Every state-changing call targets this project's `origin` and nothing else. A broad or logged-in identity is capability, not permission - a token that *can* reach another repository does not authorize writing to it. Writing to any other repository needs explicit, per-session human permission for that specific repository, and a "harmless test" write is still a write, so there is no probe exception. Reads from anywhere are fine. - **Never fabricate, guess, or reuse an identifier passed to a write.** Every id a state-changing call consumes - a node id, a numeric id, a thread or comment id - is captured from a live query in the **same** session into a variable and passed from there. Do not hand-type an id, guess it, recall it from memory or an earlier session, or copy it from documentation or an example. Ids commonly resolve **globally**, so a wrong-but-valid id does not fail - it writes to the wrong target, in someone else's repository. If a query returns no id, stop rather than invent one to proceed. -- **A write is never a probe, and a write's output is never suppressed.** Never fire a state-changing call to see whether it works: decide it should happen, make it happen, and read the result. Never append output-discarding redirection or a force-success tail to a mutation (for example `>/dev/null`, `2>/dev/null`, `&>/dev/null`, `|| true`, `|| :`, `|| echo`) - the write's output is exactly what must be read. A write that appears to fail is **verified, not assumed harmless** - the operation may have succeeded on the server while the client reported an error - so confirm the actual state before retrying or moving on. +- **A write is never a probe, and a write's output is never suppressed.** Never fire a state-changing call to see whether it works: decide it should happen, make it happen, and read the result. Never append output-discarding redirection or a force-success tail to a mutation (for example `>/dev/null`, `2>/dev/null`, `&>/dev/null`, `|| true`, `|| :`, `|| echo`) - the write's output is exactly what must be read. A write that appears to fail is **verified, not assumed harmless** - the operation may have succeeded on the server while the client reported an error - so confirm the actual state before retrying or moving on. The ban targets hiding a *failure*. An ad-hoc call's response is the only signal you get, so `>/dev/null 2>&1`, `|| true`, or `|| echo` - which swallow the error stream or force success - are never acceptable on one. A committed script under `set -e` is a narrow exception: it may send a write's *stdout* to `/dev/null` to drop the success-response noise, because stderr stays visible and a failed write still aborts loudly (`repo-config/configure.sh` does exactly this). The exception is stdout-only suppression inside a reviewed, fail-loud script, never `2>&1` or a force-success tail, and never an ad-hoc command. ## Git and Commit Rules @@ -184,13 +184,13 @@ Applies to code and workflow (`#`) comments alike. The checks that separate work actually done from work that merely reports success. Their unifying property: **every failure below is green.** A skipped job and a passing job are indistinguishable in the aggregated required check; a pattern that matches less still exits zero; a gate that stops gating still reports success. No linter, status check, or review layer catches any of them. - **A test must assert the mechanism it names.** Label each case by the behavior it proves, and satisfy yourself it would fail if that mechanism broke. A case that passes for an incidental reason - the right answer reached by the wrong path - is worse than no case, because it is later cited as evidence. -- **Gates, filters, and gate-like watchers fail loud, never narrow quietly.** A pattern that silently matches less, an allowlist that silently stops matching, or a gate that silently stops gating all report success while doing nothing. When a construct exists to notice something, make the not-noticing case produce an error or an annotation. The workflow instance of this is [`WORKFLOW.md`](./WORKFLOW.md) D8.4 (an identity allowlist used as a gate). +- **Gates, filters, and gate-like watchers fail loud, never narrow quietly.** A pattern that silently matches less, an allowlist that silently stops matching, or a gate that silently stops gating all report success while doing nothing. When a construct exists to notice something, make the not-noticing case produce an error or an annotation. An identity allowlist used as a gate, for one, must raise an error when its list stops matching, not silently pass everything through. - **Run the repo's whole lint gate before every push, not the parts that look relevant.** CI runs all of them, so a partial local run only defers the failure - and the tool most likely to catch a given change is often the one it seems least about (an edit that manipulates line endings is exactly when `editorconfig-checker` matters). The invocations are in "Running the Linters Locally"; that section documents *how* to run each, this rule is that **all** of them run. - **Editing CRLF files programmatically: `.` matches `\r` in a regex**, so a captured line keeps its carriage return and rejoining with `\r\n` yields `CRCRLF`. A text-mode rewrite has the mirror failure, silently flattening CRLF to LF. Prefer line-based edits (`splitlines(keepends=True)`) or literal replacement over regex reassembly. This is the mechanism behind the Line Endings warning above, and it is worth naming because the corruption is invisible in a rendered diff. - **Never edit an active `.code-workspace` file.** A workspace file rewritten on disk can make VS Code reload the window, and a reload destroys the running agent session's context - the work in flight is lost with nothing to catch it, and the trigger is not fully characterized (an agent's edit has caused the reload where a human's identical edit did not). Surface the needed change for the maintainer to apply by hand. -- **A green check is not evidence the work happened.** A skipped job and a passing job are indistinguishable in the aggregated required check. When a job exists to exercise something, confirm from its log that it ran and produced the output it promises. (The `changes`-job rule under "Branching Model" is this rule's instance for that one job.) +- **A green check is not evidence the work happened.** A skipped job and a passing job are indistinguishable in the aggregated required check. When a job exists to exercise something, confirm from its log that it ran and produced the output it promises. - **A workflow change is only fully exercised by CI.** Extracting a `run:` block and executing it locally validates the script and nothing else - `secrets: inherit`, `permissions:`, `needs:` wiring, and reusable-workflow inputs resolve only in a real run. -- **A review flags an instance; fix the class.** When a reviewer cites one stale claim, one silent-narrowing pattern, or one mis-worded contract, sweep for its siblings before replying. Reviewers sample; they do not enumerate. +- **A review flags an instance - fix the class.** When a reviewer cites one stale claim, one silent-narrowing pattern, or one mis-worded contract, sweep for its siblings before replying. Reviewers sample - they do not enumerate. ## PR Review Etiquette diff --git a/AUDIT.md b/AUDIT.md index 0b22e066..aadab305 100644 --- a/AUDIT.md +++ b/AUDIT.md @@ -117,7 +117,9 @@ A repo is **operational** only if every applicable check passes. A single applic Write `reports//audit.md` from [`reports/_template.md`][template]: a dimension x {letter, intent, verdict, evidence} table with `file:line` citations (WORKFLOW.md 5A style), a drift section, and a list of proposed registry/spec updates (e.g. a resolved `classificationPending`). Rank findings most severe first. -**Findings are a point-in-time snapshot - stamp them and re-verify before acting.** [`spec/audit.py`][audit-runner] prints a run stamp (`audit run | hub `) and, per repo, the exact commit it read (`@ @`). Anything derived from a run - a report, and especially an **onboarding or conformance issue** - quotes that stamp, so a reader can tell whether it still applies. A convergence issue is generated from the audit, never composed by hand: `spec/audit.py --issue ` emits a ready-to-file title and body from that repo's live findings (grouped into must-fix, converge, and could-not-verify), so the issue content cannot drift from what the audit actually found and regenerates as the repo changes. An agent picking up such an issue **re-runs the audit first and acts on the live result, not the pasted findings**: a repo moves between filing and pickup, so a stale block leads an agent to "fix" what is already fixed (re-requesting secrets that exist, attempting a no-op forward-sync). State the findings as evidence for *why* the issue was filed, never as the current state. +**Findings are a point-in-time snapshot - stamp them and re-verify before acting.** [`spec/audit.py`][audit-runner] prints a run stamp (`audit run | hub `) and, per repo, the exact commit it read (`@ @`). Anything derived from a run - a report, and especially an **onboarding or conformance issue** - quotes that stamp, so a reader can tell whether it still applies. A convergence issue is generated from the audit, never composed by hand: `spec/audit.py --issue ` emits a ready-to-file title and body from that repo's live findings (grouped into must-fix, converge, and could-not-verify), so the issue content cannot drift from what the audit actually found and regenerates as the repo changes. + +**Re-running the audit needs a full hub clone with git history.** The verbatim stale-vs-modified classification walks the canonical's history (`git log` / `git show` from the hub root), so a shallow clone or a files-only checkout cannot answer "matches a past hub revision" and those findings are unreliable there. A downstream agent verifying one finding without the full history can instead compare against the current hub canonical on `main` - the whole file for a file-level unit, or the named `## heading` block for a verbatim section - which decides current-match but not stale-vs-modified. An agent picking up such an issue **re-runs the audit first and acts on the live result, not the pasted findings**: a repo moves between filing and pickup, so a stale block leads an agent to "fix" what is already fixed (re-requesting secrets that exist, attempting a no-op forward-sync). State the findings as evidence for *why* the issue was filed, never as the current state. **Reconcile `driftNotes` in the same pass.** A registry `driftNote` records a *current* deviation from the baseline. Once the deviation is resolved the note is deleted, not left describing finished work - hand-maintained prose drifts silently otherwise. `spec/audit.py` flags this: when a repo audits clean but a note still asserts outstanding work ("pending", "not yet", "missing", "behind", ...), it raises a drift finding naming the note. diff --git a/repo-config/configure.sh b/repo-config/configure.sh index 65bba254..784e04e1 100755 --- a/repo-config/configure.sh +++ b/repo-config/configure.sh @@ -76,6 +76,13 @@ ruleset_id() { # ruleset-name -> id of the first match (empty if none); warns on echo "Failed to list rulesets for $repo (check auth and repo access)." >&2 return 1 fi + # Fail loud rather than silently narrow: a full page means the single-fetch assumption no longer holds, and + # a missed lookup would make apply create a duplicate ruleset by name. Abort so the caller stops (it treats a + # non-zero return as "stop", never as "not found"). + if [ "$(jq 'length' <<<"$out")" -eq 100 ]; then + echo "Failed for $repo: 100 rulesets returned (the per_page cap), so the single-fetch lookup is unreliable. Reduce rulesets or add pagination before applying." >&2 + return 1 + fi # shellcheck disable=SC2016 # $n is a jq --arg variable, not a shell expansion ids="$(jq -r --arg n "$1" '.[] | select(.name==$n) | .id' <<<"$out")" if [ -z "$ids" ]; then return 0; fi @@ -172,7 +179,7 @@ check_ruleset() { # payload-file - the live ruleset must match the committed pol if [ -z "$rname" ]; then fail "ruleset payload $file has no name"; return; fi id="$(ruleset_id "$rname")" if [ -z "$id" ]; then fail "ruleset '$rname' missing"; return; fi - live="$(gh api "repos/$repo/rulesets/$id")" + if ! live="$(gh api "repos/$repo/rulesets/$id")"; then fail "ruleset '$rname' - could not read live state"; return; fi want_enf="$(jq -r '.enforcement' "$file")" assert "ruleset '$rname' enforcement = $want_enf" test "$(jq -r '.enforcement' <<<"$live")" = "$want_enf" # Every rule type the committed payload declares must be present live (payload-driven, so repo-agnostic). @@ -197,7 +204,7 @@ check_ruleset() { # payload-file - the live ruleset must match the committed pol check_settings() { local live key want got private wantdisc if [ ! -e "$settings_file" ]; then fail "settings payload $settings_file missing"; return; fi - live="$(gh api "repos/$repo")" + if ! live="$(gh api "repos/$repo")"; then fail "could not read repository settings"; return; fi # Static settings, driven from settings.json so the check never drifts from the file - add a key there and # it is audited here automatically. while IFS=$'\t' read -r key want; do