From 5440526c5c001a52ca87a9eeef87ab6e55ca2eb7 Mon Sep 17 00:00:00 2001 From: prode Date: Thu, 17 Sep 2026 10:41:31 -0300 Subject: [PATCH 1/2] fix(attribution): the shapes a footer collapses to, and the half no hook reaches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A session told not to sign its work does not stop signing it — it rephrases. Three escapes were measured walking past the validator, and the pull request comment was never read at all. A vendor product link is now a signature wherever it sits. The old gate asked whether the URL stood alone on its line, and a harness that kept the link and moved it into a sentence was read as prose. A product page is nobody's citation, so its position says nothing. Documentation keeps the old gate — code.claude.com and docs.anthropic.com are sources, and an argument that cannot cite the vendor's own documentation is one nobody can check. Crediting a tool in one's own words is the same claim as the footer: "thanks to", "with the help of", "powered by", "pair-programmed with", and the instrumental voice, "used Claude Code to draft the tests". The last needs the stricter test — the name has to be the verb's own object and not possessive — because the passive reading is ordinary prose about a product this repository supports. `scc validate --pr` now reads the comments and the review bodies. A commit message passes through commit-msg on the machine that wrote it; a comment is posted straight into the forge, by a tool as readily as by a person, and nothing ran when it was. The query degrades on its own, so a forge that answers about the pull request and not about its comments still gets the title and body checked. Two things the review found, both in code this change did not write but did newly expose to live traffic: `signed` accepted any unicode symbol, and `>` and `|` are Sm — so every blockquoted line and every table row opened a signature, and the rule reported "> Claude Code, Codex, and opencode", the very sentence its own paragraph promises never to report. Measured over this repository's 147 pull request bodies and comments: 56 hits before, of which two were false — a review bot's quoted file list and a table row from #6 — and 54 after, every one a real signature. The symbol now has to be non-ASCII, which is what a signature's decoration actually is. And a file is not a name: CLAUDE.md is a path the vocabulary matches on, so a line naming it read as somebody signing the work. `clip` also escapes now. It formats the offending line into a finding that finding.Set.Report prints to a real terminal on every push, and under --pr that line is written by anybody who can comment. A raw ESC surviving the trip is a control sequence the reader's emulator obeys — enough to scroll a finding off the screen and paint a clean report over it, which is the one lie this validator exists to prevent. Verified: go build, golangci-lint run, golangci-lint fmt --diff, and scc validate --checks --pr — 12 validators, 0 findings; 1001 tests at 86.2%. The corpus measurement above is the evidence for the false-positive claim. --- .claude/rules/delivery.md | 8 +- .claude/scc-manifest.json | 50 +++--- docs/notes.md | 3 + internal/assets/assets.go | 2 +- internal/assets/assets_test.go | 2 +- internal/assets/templates/rules/delivery.md | 8 +- internal/attribution/attribution.go | 154 ++++++++++++++++-- internal/attribution/attribution_test.go | 171 +++++++++++++++++++- internal/git/git.go | 65 ++++++++ internal/git/repo_test.go | 44 +++++ internal/validate/attribution.go | 65 +++++++- internal/validate/attribution_pr_test.go | 171 ++++++++++++++++++++ 12 files changed, 684 insertions(+), 59 deletions(-) create mode 100644 internal/validate/attribution_pr_test.go diff --git a/.claude/rules/delivery.md b/.claude/rules/delivery.md index c9b110b..20b3e88 100644 --- a/.claude/rules/delivery.md +++ b/.claude/rules/delivery.md @@ -37,10 +37,10 @@ Once the last task is done: 3. **Commit and push.** Conventional Commits, written from the diff and the spec. 4. **Open the PR.** Body: what changed, which spec or plan, how it was verified. -**The work is the user's, and the record says so.** No `Co-Authored-By` for an assistant, -no session link, no "generated with" footer or badge, no naming of a model, vendor or -harness — not in a commit message, not in a PR title or body. The `commit-msg` hook -rejects it as you write it, `SCC_SKIP_HOOKS=1` is not the fix, `--pr` reads the PR body. +**The work is the user's, and the record says so.** No `Co-Authored-By` for an assistant, no +vendor link, no "generated with" footer or badge, no crediting a tool ("thanks to", "with the +help of", "used X to"), no naming a model, vendor or harness — not in a commit, a PR title, +body or comment. `commit-msg` rejects a commit, `--pr` the forge; `SCC_SKIP_HOOKS=1` is no fix. **The gates are commands, not claims.** What this workspace recorded for build, format, lint and test; the last reads back `{"total": N, "coverage": P}` and is held to a floor. diff --git a/.claude/scc-manifest.json b/.claude/scc-manifest.json index a2ec126..75a5780 100644 --- a/.claude/scc-manifest.json +++ b/.claude/scc-manifest.json @@ -10,62 +10,62 @@ { "hash": "08141d3f57fbe10f46ea557de9969fe06f96e4aceca90cd14079aea0162e4a73", "path": ".claude/agents/scc-code-review.md", - "version": "32" + "version": "35" }, { "hash": "86ae07c5864d0c38932cf9781e5b0ac5d6f6e36925577efdef646e779abbe65b", "path": ".claude/agents/scc-security-review.md", - "version": "32" + "version": "35" }, { "hash": "f3132fd6be0b859b5ffb063e22ba3b840226c5bfdde2613cd21993ff12afa477", "path": ".claude/rules/artifacts.md", - "version": "32" + "version": "35" }, { "hash": "2eb97e13f6d088379c51870ff7338ddd36d3e377486408d5a2a2d236fe05a1e1", "path": ".claude/rules/autonomy.md", - "version": "32" + "version": "35" }, { "hash": "8eee2d450faada83295a4a04e5a1267bf5f717df9e7ac7da5fddd7649c666499", "path": ".claude/rules/caveman.md", - "version": "32" + "version": "35" }, { "hash": "cfe46c6623c20b6da46c9f18ef9b35e0f554c6c4568791ff190648dda3524706", "path": ".claude/rules/code-search.md", - "version": "32" + "version": "35" }, { - "hash": "3ad4d9513a212bcced1b99db171a9ed53ccb4be38063f345ae722e6c8f36fa20", + "hash": "3bb9d1336dd9478c76f1957167bbfddba5aa5adf97908b0791caaf75e96c446f", "path": ".claude/rules/delivery.md", - "version": "32" + "version": "35" }, { "hash": "ffca716ea25746d8e100ba3a7a7b884907736adebf3a7131b989fa4e9fcd0d80", "path": ".claude/rules/knowledge-base.md", - "version": "32" + "version": "35" }, { "hash": "59bb916a2002d4176353463b017c366868192cf95c80fd1dce74265188b738c3", "path": ".claude/rules/ladder.md", - "version": "32" + "version": "35" }, { "hash": "79d5cbf1ab7fd09d26e9ea9119f3f283615467dbc35d3a4b1d4a030044b3598f", "path": ".claude/rules/methodology.md", - "version": "32" + "version": "35" }, { "hash": "7dcbf5cd9d2e9e8d08aed4eeaa96ee28af85da1d6de51fc35870b8430e30f331", "path": ".claude/rules/notes.md", - "version": "32" + "version": "35" }, { "hash": "52c5033fc427445b4e669c7865dbd8d5c2fc27d57f7200aeeb2cc48bc8e050d3", "path": ".claude/rules/prior-art.md", - "version": "32" + "version": "35" }, { "hash": "ae0030df2e1f053642a84953648e1d6038b0f0fbae831edf1c349badfb00485c", @@ -75,62 +75,62 @@ { "hash": "c8502ace5978c7b9c0543c49ebb5c9550584b1a3405e12c0217b595647e27722", "path": ".claude/rules/routing.md", - "version": "32" + "version": "35" }, { "hash": "0b86b1a73769c6ebd37c338c5ec3cbee8dda70b21d43b27a1c95eebc5ee70a9f", "path": ".claude/rules/specs.md", - "version": "32" + "version": "35" }, { "hash": "d64a118d493424b10ad8af24861083c34541f7946a92dae92fb153b0ab739c26", "path": ".claude/rules/tasks.md", - "version": "32" + "version": "35" }, { "hash": "403174a8133018b54adc69c3047c46f8ff603b8a91759c62da1e7ca6b7f45463", "path": ".claude/rules/verification.md", - "version": "32" + "version": "35" }, { "hash": "cce99457f203766837ec2b8165a9370924178d9ee377c7848dba65880461cf3f", "path": ".claude/skills/scc-adr/SKILL.md", - "version": "32" + "version": "35" }, { "hash": "fc2ecb43e7e3c157082dc359d1d0fb20c99fdb6fb774cfcead3c8c0961fef922", "path": ".claude/skills/scc-codewiki/SKILL.md", - "version": "32" + "version": "35" }, { "hash": "60e80dcee7e68498679735ac40812ac90641340fef6bd75d3e7de64d209f8716", "path": ".claude/skills/scc-glossary/SKILL.md", - "version": "32" + "version": "35" }, { "hash": "9d21e953cce950ccf495810c94411a98706780b2b8d38817e3ce13bfc0c74625", "path": ".claude/skills/scc-init/SKILL.md", - "version": "32" + "version": "35" }, { "hash": "2385f307f96cc4261e44d1a0d49bb39d6a5842a3205cee317e3228ced0f6aeb6", "path": ".claude/skills/scc-plan-run/SKILL.md", - "version": "32" + "version": "35" }, { "hash": "3d3773f869d73beeab4c2061397c9ad81faf6d4c11d1ef423470aca432c5a9e1", "path": ".claude/skills/scc-prd/SKILL.md", - "version": "32" + "version": "35" }, { "hash": "42dff0d55f15c60da604f62fc1dfe8af5b73de2e2497553411447aae57698e1b", "path": ".claude/skills/scc-stack/SKILL.md", - "version": "32" + "version": "35" }, { "hash": "aa53b7ddf701ea495d68ea12aa23a6d352c202edc0e308b7218a1bf685b09f21", "path": ".claude/skills/scc-wiki/SKILL.md", - "version": "32" + "version": "35" }, { "hash": "9dc2f234756bfdc79aa5697e8470f80766e68a0e72b6cda98107e25ea8a9427a", diff --git a/docs/notes.md b/docs/notes.md index ce667af..0c910b9 100644 --- a/docs/notes.md +++ b/docs/notes.md @@ -49,3 +49,6 @@ over this file answers with the example above as well as with the notes. --> - n-0009 2026-09-15 #gotcha @internal/mdblock/mdblock.go @internal/rtk/rtk.go @internal/mdscan/mdscan.go — a marker written in prose or a code span is not a block — mdblock and rtk.ForeignBlock search mdscan.Mask, and the entry file that documents the markers is why - n-0010 2026-09-15 #ceiling @internal/mdscan/mdscan.go — mdscan.Mask hides fenced blocks and inline code spans only — a marker in a 4-space-indented block or a blockquote still reads as a live block; lift it by teaching Mask those two constructs - n-0011 2026-09-15 #gotcha @internal/artifact/artifact.go — artifact.Within must resolve both sides the same way — resolving only the root made a not-yet-created path inside the workspace read as an escape on macOS and on Windows 8.3 paths + +- n-0013 2026-09-17 #ceiling @internal/finding/finding.go @internal/render/render.go — finding.Set.Report prints every Message to stderr unescaped - only validate/attribution clip strips control runes, so any validator embedding untrusted text inherits terminal-escape injection; lift it by escaping in render +- n-0014 2026-09-17 #gotcha @internal/scaffold @internal/manifest — scc update writes the manifest only when it applies a file, so a managed file already byte-identical to the next template keeps its old hash - and at the version after that it compares as edited and is never updated again; deleting the file and re-running update is the way back diff --git a/internal/assets/assets.go b/internal/assets/assets.go index ecf149f..a7a2617 100644 --- a/internal/assets/assets.go +++ b/internal/assets/assets.go @@ -283,7 +283,7 @@ import ( // `/home/node/.claude` is created and chowned in the image: without it the first // write of every session was `EACCES ... mkdir '/home/node/.claude/session-env'` and // the login it could not store read back as "not logged in". -const Version = "33" +const Version = "35" // The embedded tree. "all:" so nothing is silently dropped for having a name the // default embed pattern skips. diff --git a/internal/assets/assets_test.go b/internal/assets/assets_test.go index f5a016d..c5f386b 100644 --- a/internal/assets/assets_test.go +++ b/internal/assets/assets_test.go @@ -1075,7 +1075,7 @@ func (e errNoSuchRule) Error() string { // uses wherever a change is cheap to make and expensive to make by accident. func TestTheTemplateVersionMovesWithTheTemplates(t *testing.T) { // Bump Version, then replace this with the digest the failure prints. - const fingerprint = "5f4a989994ef836fd039f1f188f1027ee4924164a0ae7a6ebd30ce345f6f6353" + const fingerprint = "ca004345d7bd327f81c7361b2ceca205d8e2e1609e4b527118b1e1a7e9c65a05" sum := sha256.New() // Version goes into the hash, and without it this test does not do the job its diff --git a/internal/assets/templates/rules/delivery.md b/internal/assets/templates/rules/delivery.md index c9b110b..20b3e88 100644 --- a/internal/assets/templates/rules/delivery.md +++ b/internal/assets/templates/rules/delivery.md @@ -37,10 +37,10 @@ Once the last task is done: 3. **Commit and push.** Conventional Commits, written from the diff and the spec. 4. **Open the PR.** Body: what changed, which spec or plan, how it was verified. -**The work is the user's, and the record says so.** No `Co-Authored-By` for an assistant, -no session link, no "generated with" footer or badge, no naming of a model, vendor or -harness — not in a commit message, not in a PR title or body. The `commit-msg` hook -rejects it as you write it, `SCC_SKIP_HOOKS=1` is not the fix, `--pr` reads the PR body. +**The work is the user's, and the record says so.** No `Co-Authored-By` for an assistant, no +vendor link, no "generated with" footer or badge, no crediting a tool ("thanks to", "with the +help of", "used X to"), no naming a model, vendor or harness — not in a commit, a PR title, +body or comment. `commit-msg` rejects a commit, `--pr` the forge; `SCC_SKIP_HOOKS=1` is no fix. **The gates are commands, not claims.** What this workspace recorded for build, format, lint and test; the last reads back `{"total": N, "coverage": P}` and is held to a floor. diff --git a/internal/attribution/attribution.go b/internal/attribution/attribution.go index bc73813..229ad0a 100644 --- a/internal/attribution/attribution.go +++ b/internal/attribution/attribution.go @@ -51,12 +51,18 @@ const ( // address. It is separate from the other two because it survives them: a // footer rewritten by hand usually keeps the URL. // - // A session link and a no-reply address fire wherever they sit — nobody cites - // either to explain a change. Any other vendor URL has to be alone on its line, - // because documentation lives on those hosts and an argument that cannot cite - // the vendor's own docs is one nobody can check. That gate costs a real miss: - // a bare host inside a sentence is no longer reported, which is this package's - // standing trade. + // It fires wherever the URL sits — inside prose, inside a parenthesis, inside + // an HTML comment — and the one thing that buys an exemption is being + // *documentation*. That split replaces an earlier one that asked whether the + // URL was alone on its line, and it was measured rather than argued: a harness + // told not to sign its work wrote the product link into a sentence, and a rule + // that only looked at lines standing by themselves read it as prose. A product + // page is nobody's citation, so the position it sits in says nothing. + // + // Documentation keeps the old gate. `code.claude.com/docs/…` is a source, and + // a technical argument that cannot cite the vendor's own documentation is one + // nobody can check — so a docs URL is reported only when it stands alone, + // where it is the footer's surviving half rather than a reference. RuleLink = "assistant-link" // RuleBadge is a Markdown link or image whose label or target names an // assistant — the `[Claude Code](…)` half of the footer the harnesses append. @@ -84,17 +90,40 @@ const ( var ( // A git trailer: a known key at the start of a line, with a value. trailer = regexp.MustCompile(`(?i)^[ \t]*(co-?authored-by|assisted-by|generated-by|signed-off-by)[ \t]*:[ \t]*(\S.*?)[ \t]*$`) - // The footer phrase. It is only half a shape: the caller requires the name to + // The footer phrase, and every other way a line hands this change to whoever + // it names next. It is only half a shape: the caller requires the name to // follow it on the same line, so a sentence that happens to say "written by" // about something else stays prose. - footer = regexp.MustCompile(`(?i)\b(?:generated|created|authored|written|made|built|produced|drafted)\s+(?:with|by|using|via)\b`) - // A session link, a vendor's own page, or a vendor no-reply address, which is - // a signature on its own: nobody cites any of these to explain a change. + // + // Three families, and the second and third are what a session writes once it + // has been told not to write the first. "Generated with" is the harness + // default; "with the help of" and "thanks to" are what it collapses to when + // somebody rephrases it in their own words; and the verb list is wider than + // the footer's because the sentence being rewritten is about this work, so any + // verb for making it will do. + footer = regexp.MustCompile(`(?i)\b(?:` + + `(?:generated|created|authored|written|made|built|produced|drafted|implemented|developed|refactored|scaffolded|assembled|composed|prepared|completed|reviewed)\s+(?:with|by|using|via)` + + `|(?:with|using)\s+(?:the\s+)?(?:help|assistance|support|aid)\s+(?:of|from)` + + `|thanks\s+to|powered\s+by|driven\s+by|came\s+from|courtesy\s+of` + + `|pair(?:ed|[- ]programmed)?\s+with|in\s+(?:collaboration|partnership)\s+with` + + `)\b`) + // The other half of crediting: the assistant as the instrument rather than the + // agent — "used Claude Code to draft the tests". The name has to be what + // follows the verb, not merely somewhere after it, because the passive reading + // of the same words is ordinary technical prose: "the settings format used by + // Claude Code" describes a product, it does not hand it the work. + useVerb = regexp.MustCompile(`(?i)\b(?:used|using|ran)\s+(?:the\s+|a\s+|an\s+|my\s+|our\s+)?`) + // A session link, a vendor's own page, or a vendor no-reply address. // // Whole hosts, rather than the one path today's footer happens to use. // claude.com/claude-code is the spelling that walked past a rule written // around claude.ai, and the next rewording moves the path again. link = regexp.MustCompile(`(?i)\b(?:https?://)?(?:[\w-]+\.)*(?:claude\.(?:ai|com)|anthropic\.com|chatgpt\.com|openai\.com|cursor\.(?:com|sh)|copilot\.microsoft\.com|codeium\.com|windsurf\.com|sourcegraph\.com/cody)(?:/\S*)?|\b[\w.+-]+@(?:anthropic|openai)\.com\b`) + // What makes a vendor URL citable: a documentation host, or a path whose first + // meaningful segment is documentation. Everything else on a vendor host is a + // product page, and nobody cites a product page to explain a change. + docHost = regexp.MustCompile(`(?i)^(?:https?://)?(?:docs?|developers?|platform|api|help|support|code)\.`) + docPath = regexp.MustCompile(`(?i)^(?:https?://)?[^/\s]+/(?:[\w-]+/){0,3}?(?:docs?|reference|api|guides?|blog|news|engineering|research|help|support|changelog|papers?)(?:[/#?]|$)`) // A Markdown link or image: a label in brackets and a target in parentheses, // with an optional leading bang. Both halves are measured, so a badge survives // having either one of them rewritten. @@ -208,10 +237,13 @@ func scanLine(line string, last bool) (Hit, bool) { if at := footer.FindStringIndex(line); at != nil && Names(line[at[1]:]) { return Hit{Rule: RuleFooter, Match: strings.TrimSpace(line)}, true } + if instrumental(line) { + return Hit{Rule: RuleFooter, Match: strings.TrimSpace(line)}, true + } if m := badge.FindStringSubmatch(line); m != nil && (Names(m[1]) || Names(m[2])) && signsAlone(line, m[0]) { return Hit{Rule: RuleBadge, Match: strings.TrimSpace(m[0])}, true } - if m := link.FindString(line); m != "" && (session.MatchString(m) || strings.Contains(m, "@") || signsAlone(line, m)) { + if m := link.FindString(line); m != "" && linkSigns(line, m) { return Hit{Rule: RuleLink, Match: m}, true } if m, ok := mention(line, last); ok { @@ -220,6 +252,64 @@ func scanLine(line string, last bool) (Hit, bool) { return Hit{}, false } +// instrumental reports a line that uses an assistant to do this work — "used +// Claude Code to draft the tests", "ran Codex over the diff". +// +// It is the footer rule's other voice, and it is separated from it because it +// needs a stricter test. The footer phrase can look anywhere after itself for the +// name, since "generated with" is already about this change. "Used" is not: the +// same verb in the passive describes a product rather than crediting one, and +// this repository's own history writes that sentence — "the settings format used +// by Claude Code" is prose about a harness scc supports. +// +// So the name has to be the verb's own object — the very next word, article +// aside — and it must not be possessive. "Used by Claude Code" puts a preposition +// there and is the passive; "using Claude Code's hook surface" is a sentence +// about a hook surface; "using Claude Code to write this" hands over the work. +// Those two tests are cheap, and they stand in for a distinction that would +// otherwise need a parser. +func instrumental(line string) bool { + at := useVerb.FindStringIndex(line) + if at == nil { + return false + } + rest := strings.Fields(line[at[1]:]) + if len(rest) == 0 || !Names(rest[0]) { + return false + } + return !strings.ContainsAny(firstWords(rest, 3), "'’") +} + +// firstWords is the opening of a fragment, punctuation kept. Kept rather than +// stripped because the punctuation is what instrumental reads: an apostrophe +// three words in is the difference between a credit and a description. +func firstWords(f []string, n int) string { + if len(f) > n { + f = f[:n] + } + return strings.Join(f, " ") +} + +// linkSigns decides whether a vendor URL on this line is a signature. +// +// A session link and a no-reply address always are: nobody cites either to +// explain a change. Documentation is the one exemption, and it keeps the older +// "alone on its line" gate, because a docs URL in a sentence is a source and the +// same URL by itself at the end of a message is the footer with the words taken +// off. Everything else on a vendor host is a product page, and a product page is +// a signature wherever it sits — in prose, in a parenthesis, or inside an HTML +// comment, which is where one was measured getting through. +func linkSigns(line, m string) bool { + switch { + case session.MatchString(m), strings.Contains(m, "@"): + return true + case docHost.MatchString(m), docPath.MatchString(m): + return signsAlone(line, m) + default: + return true + } +} + // mention reports a line that is nothing but an assistant's name, in the place a // signature goes. // @@ -247,6 +337,13 @@ func mention(line string, last bool) (string, bool) { for _, w := range words { low := strings.ToLower(w) switch { + // A file is not a name, and this repository is the reason the distinction + // had to be drawn: `CLAUDE.md` is a path the vocabulary matches on, so a + // last line naming the file — which a review bot writes and a commit body + // listing what it touched writes too — was read as somebody signing it. + // The line says something else, so it is not a bare mention. + case isFilename(low): + return "", false case Names(low): named = true case filler[low], isVersion(low): @@ -268,13 +365,46 @@ func mention(line string, last bool) (string, bool) { // of harnesses starts. A signature's decoration and a list's bullet look alike // enough that guessing between them is how this rule would earn its first false // positive. +// +// Saying that was not enough to make it true, and the gap was measured rather +// than argued. `unicode.IsSymbol` returns true for `>`, which is Sm — so every +// blockquoted line opened a signature, and `> Claude Code, Codex, and opencode` +// was reported: the very sentence the rule above promises never to report, quoted +// back. A review bot's comment is written entirely in blockquotes, and `--pr` now +// reads comments, so the paragraph and the code had to be made to agree. +// +// The symbol has to be non-ASCII. That is what a signature's decoration actually +// is — the robot, a sparkle, a trademark — while ASCII's symbol block is Markdown +// punctuation: `>`, `|`, `=`, `+`, `^`, the backtick, `~`. The one ASCII +// character kept is the tilde, named above and never a Markdown opener at the +// start of a line. func signed(line string) bool { for _, r := range strings.TrimLeft(line, " \t") { - return unicode.IsSymbol(r) || r == '—' || r == '–' || r == '~' + return (unicode.IsSymbol(r) && r > unicode.MaxASCII) || r == '—' || r == '–' || r == '~' } return false } +// isFilename reports a token that is a file rather than a name: a dot with a +// short alphabetic tail, which is an extension. +// +// It sits beside isVersion because it is the same kind of test — a shape rather +// than a word — and it is deliberately narrow. A model name keeps its dots +// ("claude-3.5-sonnet" ends in "5-sonnet", which is not an extension), and a +// vendor host ending in one ("claude.ai") is the link rule's to report and is +// reached before this. +func isFilename(w string) bool { + i := strings.LastIndexByte(w, '.') + if i <= 0 || i == len(w)-1 { + return false + } + ext := w[i+1:] + if len(ext) > 4 { + return false + } + return strings.IndexFunc(ext, func(r rune) bool { return !unicode.IsLetter(r) }) < 0 +} + // isVersion accepts the scraps of a model name that are not words: "5", "4.5", // "v2", "1m". It sits beside filler rather than in it because it is a shape too. func isVersion(w string) bool { diff --git a/internal/attribution/attribution_test.go b/internal/attribution/attribution_test.go index 06b3f4d..39ba834 100644 --- a/internal/attribution/attribution_test.go +++ b/internal/attribution/attribution_test.go @@ -115,10 +115,6 @@ func TestTheFooterRewrittenIsStillCaught(t *testing.T) { {"badge named by target", "[the tool](https://claude.ai/code)", RuleBadge}, {"image badge", "![Claude](https://img.example/badge.svg)", RuleBadge}, {"vendor page", "https://www.anthropic.com/claude-code", RuleLink}, - // Alone on the line, which is where a footer's surviving URL sits. A vendor - // host *inside* a sentence is now a deliberate miss: documentation lives on - // these domains, and this package trades a miss for a false positive every - // time — one wrong finding teaches the reader to disbelieve the other ten. {"vendor host alone", "claude.com/claude-code", RuleLink}, {"session link mid-sentence", "picked up from https://claude.ai/code/session_01abc in passing", RuleLink}, {"bare name under a symbol", "\U0001F916 Claude Code", RuleMention}, @@ -196,3 +192,170 @@ func TestBadgeTellsACitationFromASignature(t *testing.T) { } } } + +// TestAProductPageIsASignatureWhereverItSits is the measured escape this rule was +// widened for. A harness told not to sign its work kept the link and moved it into +// a sentence, and the gate that asked whether the line stood alone read that as +// prose. A product page is nobody's citation, so where it sits says nothing about +// what it is. +func TestAProductPageIsASignatureWhereverItSits(t *testing.T) { + for _, tc := range []struct { + name string + line string + }{ + {"in prose", "See https://claude.com/claude-code for the tool that helped here."}, + {"in an HTML comment", ""}, + {"in a parenthesis", "Part of the work happened elsewhere (claude.com/claude-code)."}, + {"bare host mid-sentence", "Some of the tests came out of claude.com and were tidied by hand."}, + {"trailing punctuation", "Tooling: https://claude.com/claude-code."}, + {"a vendor page that is not the product", "written up at https://www.anthropic.com/claude in more detail"}, + } { + t.Run(tc.name, func(t *testing.T) { + hits := Scan("fix: thing\n\n" + tc.line) + if len(hits) != 1 || hits[0].Rule != RuleLink { + t.Fatalf("got %+v, want one %s hit", hits, RuleLink) + } + }) + } +} + +// TestDocumentationStaysCitable is the other side of that trade, and the more +// important one. An argument that cannot cite the vendor's own documentation is +// one nobody can check, and a validator that fires on scc's own pull requests is +// the worst bug in this product. +func TestDocumentationStaysCitable(t *testing.T) { + for _, line := range []string{ + "Anthropic's own guidance at https://code.claude.com/docs/en/devcontainer warns about exfiltration.", + "The event list is at code.claude.com/docs/en/hooks, which is where the stage names come from.", + "Measured against https://docs.anthropic.com/en/api/messages and the SDK's own defaults.", + "The post at https://www.anthropic.com/news/claude-opus-5 is where the figure comes from.", + "Rate limits: platform.openai.com/docs/guides/rate-limits, for the comparison.", + } { + if hits := Scan("fix: thing\n\n" + line); len(hits) != 0 { + t.Errorf("citation reported as a signature: %q → %+v", line, hits) + } + } + + // It is still reported standing by itself, which is where a footer's surviving + // URL lands once the words in front of it are gone. + if hits := Scan("fix: thing\n\nhttps://code.claude.com/docs/en/overview"); len(hits) != 1 { + t.Errorf("a docs URL alone on its line is the footer with the words off: got %+v", hits) + } +} + +// TestCreditingAToolIsAttribution covers the phrasings a session reaches for once +// it has been told not to write "Generated with" — the same claim in somebody's +// own words. +func TestCreditingAToolIsAttribution(t *testing.T) { + for _, line := range []string{ + "Part of the work used Claude Code to draft the tests.", + "Some of this came from Claude Code.", + "Written with the help of Claude Code.", + "Thanks to Claude for the refactor.", + "Powered by Codex.", + "Pair-programmed with Claude Opus 5.", + "Scaffolded with opencode, then finished by hand.", + "Reviewed by Claude before it was pushed.", + "Using Claude Code, the tests were rewritten from the requirement.", + } { + hits := Scan("fix: thing\n\n" + line) + if len(hits) != 1 || hits[0].Rule != RuleFooter { + t.Errorf("credit not reported: %q → %+v", line, hits) + } + } +} + +// TestTalkingAboutAHarnessIsNotCreditingIt is where the widened rule is weakest, +// and the bar is this repository's own history: fifteen of its commits discuss +// Claude Code as the harness scc scaffolds for, and not one of them credits it +// with the work. Every line here is taken from that history or written in its +// register. +func TestTalkingAboutAHarnessIsNotCreditingIt(t *testing.T) { + for _, text := range []string{ + "refactor(assets): drop the command files on Claude Code, move the prefix onto the skill", + "feat(assets): scaffold the methodology into Codex and opencode, not only Claude Code", + "fix: x\n\nClaude Code registers a skill at skills//SKILL.md as / by itself.", + "fix: x\n\nThe settings format used by Claude Code is JSON, and scc splices into it.", + "fix: x\n\nUsing Claude Code's hook surface, the Stop stage can report findings.", + "fix: x\n\nThe rules are loaded by Claude Code at launch with the same priority as CLAUDE.md.", + "feat(launch): sandbox by default, with a dev container for Windows", + "fix: x\n\nThe image carries Claude Code, scc, codegraph and rtk, pinned to build ARGs.", + "fix: x\n\nMeasured on a freshly scaffolded Claude Code workspace: 47KB in every request.", + } { + if hits := Scan(text); len(hits) != 0 { + t.Errorf("Scan(%q) reported %+v, want nothing", text, hits) + } + } +} + +// TestMarkdownStructureIsNotASignaturePosition is the measured regression, and +// the corpus is this repository's own pull requests: every body, every issue +// comment and every review comment. Two lines were reported before the position +// gate was made to mean what its own paragraph claimed — a quoted bullet in a +// review bot's file list, and a table row from PR #6 comparing the three +// harnesses. `unicode.IsSymbol` is true for `>` and `|`, so Markdown's own +// structure opened a signature. +// +// Reading comments is what made this live traffic: a review bot writes entirely +// in blockquotes. +func TestMarkdownStructureIsNotASignaturePosition(t *testing.T) { + for _, line := range []string{ + "> * `CLAUDE.md`", + "> Claude Code registers a skill as a command by itself.", + "| | Claude Code | Codex | opencode |", + "| Claude Code | yes |", + "= Claude Code", + "+ Claude Code", + "^ Claude Code", + } { + // Not the last line, which is a signature's other position and tested + // below: this is about the decoration in front of it. + if hits := Scan("fix: thing\n\n" + line + "\n\nand then some prose."); len(hits) != 0 { + t.Errorf("Markdown structure read as a signature: %q → %+v", line, hits) + } + } +} + +// And the decoration that really does open a signature still does. Every one of +// these is non-ASCII or the typographic dash the rule always named. +func TestRealDecorationStillOpensASignature(t *testing.T) { + for _, line := range []string{ + "🤖 Claude Code", + "✨ Claude Opus 5", + "— Claude Code", + "– via Codex", + "~ Claude", + } { + hits := Scan("fix: thing\n\n" + line + "\n\nand then some prose.") + if len(hits) != 1 || hits[0].Rule != RuleMention { + t.Errorf("signature not reported: %q → %+v", line, hits) + } + } +} + +// TestAFileIsNotAName is the other half of the same corpus result. `CLAUDE.md` is +// a path the vocabulary matches on, so a last line naming the file — which a +// review bot writes, and so does a commit body listing what it touched — read as +// somebody signing the work. +func TestAFileIsNotAName(t *testing.T) { + for _, line := range []string{ + "CLAUDE.md", + "🤖 CLAUDE.md", + "claude.md", + "— AGENTS.md and CLAUDE.md", + } { + if hits := Scan("fix: thing\n\n" + line); len(hits) != 0 { + t.Errorf("a filename read as a signature: %q → %+v", line, hits) + } + } + // A model name keeps its dots and is still a name: the gate is an extension, + // not a full stop. + for _, line := range []string{ + "🤖 claude-3.5-sonnet", + "— Claude Opus 5", + } { + if hits := Scan("fix: thing\n\n" + line); len(hits) != 1 { + t.Errorf("model name not reported: %q → %+v", line, hits) + } + } +} diff --git a/internal/git/git.go b/internal/git/git.go index 914e214..2413e1d 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -278,6 +278,71 @@ func viewPR(dir, arg string) (PR, error) { }, nil } +// PRComment is one thing somebody wrote on a pull request after it was opened — +// an issue comment or the body of a review. +// +// It is separate from PR.Body because it is the one part of the record nothing +// else can reach. A commit message passes through commit-msg on the machine that +// wrote it; a pull request body is at least typed once by whoever opened it; a +// comment is posted straight into the forge at any point afterwards, by a tool as +// readily as by a person. Whatever rule binds the first two binds this. +// +// Author is the login, which is how a reader finds the comment again — the URL is +// exact but unreadable in a terminal, so both are kept. +type PRComment struct { + Author string `json:"author"` + URL string `json:"url"` + Body string `json:"body"` +} + +// PRComments is everything written on a pull request after it was opened: the +// issue comments and the bodies of the reviews, in that order. +// +// A number of 0 means the pull request for the checked-out branch, which is gh's +// own default and the same convention CurrentPR follows. +// +// It is a second call rather than another field on PR, and that is deliberate: +// spec sync asks LookPR about every spec in the workspace, and it has no use for +// comment bodies. Only the attribution check pays for these, and only under +// `scc validate --pr`. +func PRComments(dir string, number int) ([]PRComment, error) { + args := []string{"pr", "view"} + if number > 0 { + args = append(args, strconv.Itoa(number)) + } + out, err := run(GHBin, dir, append(args, "--json", "comments,reviews")...) + if err != nil { + return nil, err + } + var raw struct { + Comments []rawComment `json:"comments"` + Reviews []rawComment `json:"reviews"` + } + if err := json.Unmarshal([]byte(out), &raw); err != nil { + return nil, err + } + var all []PRComment + for _, c := range append(append([]rawComment{}, raw.Comments...), raw.Reviews...) { + // A review with no body is an approval click, and there is nothing in it to + // read. Dropping it here keeps the caller from reporting an empty record. + if strings.TrimSpace(c.Body) == "" { + continue + } + all = append(all, PRComment{Author: c.Author.Login, URL: c.URL, Body: c.Body}) + } + return all, nil +} + +// rawComment is gh's shape for both lists, which differ only in fields neither +// caller reads. +type rawComment struct { + Author struct { + Login string `json:"login"` + } `json:"author"` + URL string `json:"url"` + Body string `json:"body"` +} + // Commit is one commit's identity and its message in full. // // Message is the whole thing — subject, body and trailers — because the trailers diff --git a/internal/git/repo_test.go b/internal/git/repo_test.go index 689d51e..a3be3e6 100644 --- a/internal/git/repo_test.go +++ b/internal/git/repo_test.go @@ -503,3 +503,47 @@ func TestViewPRReadsWhatTheForgeSays(t *testing.T) { t.Error("LookPR accepted output that is not JSON") } } + +// A comment is the half of the pull request that nothing else can reach: no hook +// runs when a tool posts one. The query reads the issue comments and the review +// bodies as one list, and drops a review that is an approval click with nothing +// written in it. +func TestPRCommentsReadsWhatWasWrittenAfterwards(t *testing.T) { + const body = `{"comments":[{"author":{"login":"someone"},"url":"https://example.invalid/pr/7#c1","body":"looks good"}],` + + `"reviews":[{"author":{"login":"other"},"url":"https://example.invalid/pr/7#r1","body":"one nit"},` + + `{"author":{"login":"third"},"url":"https://example.invalid/pr/7#r2","body":" "}]}` + t.Setenv("PATH", fakeBin(t, GHBin, body, 0)) + + got, err := PRComments(t.TempDir(), 7) + if err != nil { + t.Fatalf("PRComments: %v", err) + } + if len(got) != 2 { + t.Fatalf("got %d comments, want 2 (the empty review is an approval click): %+v", len(got), got) + } + // Issue comments first, then reviews, so a reader meets them in the order the + // pull request page shows them. + if got[0].Author != "someone" || got[0].Body != "looks good" { + t.Errorf("first = %+v", got[0]) + } + if got[1].Author != "other" || got[1].URL != "https://example.invalid/pr/7#r1" { + t.Errorf("second = %+v", got[1]) + } + + // The current branch's pull request is the same query with no number, which is + // gh's own default. + if _, err := PRComments(t.TempDir(), 0); err != nil { + t.Errorf("PRComments(0): %v", err) + } + + // And every way of having no answer is an error the caller turns into silence, + // never a half-read list. + t.Setenv("PATH", fakeBin(t, GHBin, "not json", 0)) + if _, err := PRComments(t.TempDir(), 7); err == nil { + t.Error("PRComments accepted output that is not JSON") + } + t.Setenv("PATH", t.TempDir()) + if _, err := PRComments(t.TempDir(), 7); err != ErrUnavailable { + t.Errorf("err = %v, want ErrUnavailable when gh is not on PATH", err) + } +} diff --git a/internal/validate/attribution.go b/internal/validate/attribution.go index dc680f1..3c8b3c7 100644 --- a/internal/validate/attribution.go +++ b/internal/validate/attribution.go @@ -3,6 +3,7 @@ package validate import ( "strconv" "strings" + "unicode" "github.com/protonspy/spec-claude-code/internal/attribution" "github.com/protonspy/spec-claude-code/internal/finding" @@ -51,7 +52,8 @@ func Attribution(root string) (*finding.Set, error) { } // AttributionPR reports the same signatures in the pull request this branch has -// open — its title and its body. +// open — its title, its body, and everything written on it since: the issue +// comments and the bodies of the reviews. // // It is the other half of the record and the half no hook can reach. A commit // message passes through `commit-msg` on the machine that wrote it; a PR body is @@ -66,7 +68,9 @@ func Attribution(root string) (*finding.Set, error) { // // Like its sibling it never returns an error. No gh, no authentication, no // network, no pull request for this branch: all of them mean there is nothing to -// check, and none of them is this validator failing. +// check, and none of them is this validator failing. The comment query degrades +// the same way and on its own — a forge that answers about the pull request and +// not about its comments still gets the title and body checked. func AttributionPR(root string) (*finding.Set, error) { set := &finding.Set{} if !git.IsRepo(root) || !git.Found(git.GHBin) { @@ -79,9 +83,29 @@ func AttributionPR(root string) (*finding.Set, error) { // Title first and body after, as one text, so a finding's line number reads // against what a person sees on the pull request page: line 1 is the title. report(set, prName(pr), clipSubject(pr.Title), pr.Title+"\n"+pr.Body) + // And then what was written on it afterwards. A comment is the one part of + // the record nothing else can reach — no hook runs when a tool posts one — and + // it is the same text under the same rule, so it is checked the same way. + // Each comment is its own text, because its line numbers are its own. + comments, err := git.PRComments(root, pr.Number) + if err != nil { + return set, nil + } + for _, c := range comments { + report(set, prName(pr)+" comment", commentBy(c), c.Body) + } return set, nil } +// commentBy names a comment the way its finding is read: by who wrote it, since a +// comment has no title and the number is already on the left of the line. +func commentBy(c git.PRComment) string { + if c.Author == "" { + return "a comment" + } + return "a comment by " + clipSubject(c.Author) +} + // prName is what a pull-request finding is filed under: the number, because it is // what `gh pr edit ` takes and the report is read in a terminal. func prName(pr git.PR) string { return "PR #" + strconv.Itoa(pr.Number) } @@ -107,16 +131,41 @@ var advice = map[string]string{ attribution.RuleMention: "a line that says nothing but the name is a signature", } -// clip keeps a finding to one readable line. A trailer is short, but a footer can -// carry a full URL and a message that wrapped the terminal would defeat the -// grouping the report is built around. +// clip keeps a finding to one readable line, and makes that line safe to print. +// +// Two jobs, and the second is not cosmetic. What is being clipped is the +// offending line itself, and under `--pr` that line arrives from a pull request +// comment — written by anybody with an account who can comment, rather than by +// whoever opened the pull request or by anybody with push access. It reaches a +// real terminal through finding.Set.Report on every `git push`, because that is +// where pre-push runs this check. A raw ESC surviving the trip is a control +// sequence the reader's emulator obeys: enough to scroll a finding off the screen +// and paint a clean report over the top of it, which is exactly the lie this +// validator exists to prevent. The subject printed beside the match goes out +// through %q and is escaped by the verb; the match goes out through %s, so it is +// escaped here. +// +// Every non-graphic rune becomes a space before the collapse — the C0 and C1 +// controls, and the format category, which is where the bidirectional overrides +// live. strings.Fields then folds the result back into the single line this +// function was always for. +// +// And the cut is by rune rather than by byte, for the same reason the escaping is +// here at all: halving a multi-byte rune puts invalid UTF-8 in the report, and +// the text is now chosen by somebody who would like it to. func clip(s string) string { const max = 64 + s = strings.Map(func(r rune) rune { + if unicode.IsGraphic(r) { + return r + } + return ' ' + }, s) s = strings.Join(strings.Fields(s), " ") - if len(s) <= max { - return s + if r := []rune(s); len(r) > max { + return string(r[:max-1]) + "…" } - return s[:max-1] + "…" + return s } // clipSubject names the commit in words, next to the short sha the finding is diff --git a/internal/validate/attribution_pr_test.go b/internal/validate/attribution_pr_test.go new file mode 100644 index 0000000..8c05a7c --- /dev/null +++ b/internal/validate/attribution_pr_test.go @@ -0,0 +1,171 @@ +package validate + +import ( + "encoding/json" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" + "unicode" + "unicode/utf8" +) + +// A pull request comment is the one part of the record nothing else reaches: no +// hook runs when a tool posts one, and the body was written long after the branch +// was pushed. `scc validate --pr` reads them for the same signatures it reads in +// the title and the body. +func TestAttributionPRReadsTheCommentsToo(t *testing.T) { + root := gitRepo(t) + const pr = `{"number":7,"state":"OPEN","headRefName":"feat/x","url":"https://example.invalid/pr/7","title":"feat: x","body":"why"}` + const comments = `{"comments":[{"author":{"login":"someone"},"url":"https://example.invalid/pr/7#c1",` + + `"body":"Part of this came from Claude Code."}],"reviews":[]}` + stubGH(t, pr, comments) + + set, err := AttributionPR(root) + if err != nil { + t.Fatalf("AttributionPR: %v", err) + } + found := set.Sorted() + if len(found) != 1 { + t.Fatalf("got %d findings, want 1: %+v", len(found), found) + } + // Filed under the pull request it is on, and named by who wrote it: a comment + // has no title, and "PR #7" alone does not say which of ten comments to edit. + if !strings.Contains(found[0].File, "#7") || !strings.Contains(found[0].Message, "someone") { + t.Errorf("finding does not say where to look: %+v", found[0]) + } +} + +// And a forge that answers about the pull request but not about its comments +// still gets the title and the body checked. The comment query degrades on its +// own, because half the record is better than none. +func TestAttributionPRSurvivesAForgeWithNoComments(t *testing.T) { + root := gitRepo(t) + const pr = `{"number":7,"state":"OPEN","headRefName":"feat/x","url":"https://example.invalid/pr/7",` + + `"title":"feat: x","body":"Generated with Claude Code"}` + stubGH(t, pr, "") + + set, err := AttributionPR(root) + if err != nil { + t.Fatalf("AttributionPR: %v", err) + } + if len(set.Sorted()) != 1 { + t.Fatalf("the body's own footer went unreported: %+v", set.Sorted()) + } +} + +// gitRepo is an empty repository, which is all IsRepo asks for. +func gitRepo(t *testing.T) string { + t.Helper() + root := t.TempDir() + cmd := exec.Command("git", "init") + cmd.Dir = root + if err := cmd.Run(); err != nil { + t.Skip("git is not on PATH") + } + return root +} + +// stubGH puts a gh on the front of PATH that answers the two queries this +// validator makes: one about the pull request, one about its comments. It tells +// them apart by the field list, which is the only thing that differs. An empty +// comments answer is a gh that exits non-zero, which is how a forge that cannot +// answer that question reports it. +func stubGH(t *testing.T, pr, comments string) { + t.Helper() + dir := t.TempDir() + if runtime.GOOS == "windows" { + fail := "exit /b 1" + if comments != "" { + fail = "echo " + comments + } + body := "@echo off\r\n" + + "echo %*| findstr /C:\"comments\" >nul\r\n" + + "if errorlevel 1 (echo " + pr + ") else (" + fail + ")\r\n" + writeStub(t, filepath.Join(dir, "gh.cmd"), body, 0o644) + } else { + fail := "exit 1" + if comments != "" { + fail = "printf '%s\n' " + quote(comments) + } + body := "#!/bin/sh\ncase \"$*\" in\n *comments*) " + fail + " ;;\n" + + " *) printf '%s\n' " + quote(pr) + " ;;\nesac\n" + writeStub(t, filepath.Join(dir, "gh"), body, 0o755) + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) +} + +func writeStub(t *testing.T, path, body string, mode os.FileMode) { + t.Helper() + if err := os.WriteFile(path, []byte(body), mode); err != nil { + t.Fatalf("WriteFile %s: %v", path, err) + } +} + +func quote(s string) string { return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" } + +// A comment body is the least trusted text this validator reads: anybody with an +// account can post one, on somebody else's pull request, and pre-push prints what +// it finds to a real terminal. A raw ESC surviving that trip is a control sequence +// the reader's emulator obeys — enough to scroll the finding away and paint a +// clean report over the top of it, which is the one lie this check exists to +// prevent. +func TestAPRCommentCannotWriteControlSequencesToTheTerminal(t *testing.T) { + root := gitRepo(t) + const pr = `{"number":7,"state":"OPEN","headRefName":"feat/x","url":"https://example.invalid/pr/7","title":"feat: x","body":"why"}` + // A screen-clearing sequence, a bell, and a right-to-left override, on a line + // that is a signature by any reading. Built from code points rather than typed, + // so the bytes under test survive every editor between here and the run — and + // marshalled, because that is how the forge hands them over. + esc, bel, rlo := string(rune(0x1b)), string(rune(0x07)), string(rune(0x202e)) + body := esc + "[2J" + esc + "[Hno findings" + bel + " generated with Claude Code" + rlo + comments, err := json.Marshal(map[string]any{ + "comments": []map[string]any{{ + "author": map[string]string{"login": "someone"}, + "url": "https://example.invalid/pr/7#c1", + "body": body, + }}, + "reviews": []any{}, + }) + if err != nil { + t.Fatalf("Marshal: %v", err) + } + stubGH(t, pr, string(comments)) + + set, err := AttributionPR(root) + if err != nil { + t.Fatalf("AttributionPR: %v", err) + } + found := set.Sorted() + if len(found) != 1 { + t.Fatalf("got %d findings, want 1: %+v", len(found), found) + } + for _, r := range found[0].Message { + if !unicode.IsGraphic(r) && r != ' ' { + t.Fatalf("finding carries %U to the terminal: %q", r, found[0].Message) + } + } + // And the line is still reported. Escaping it must not cost the reader the + // evidence it was written to show them. + if !strings.Contains(found[0].Message, "Claude Code") { + t.Errorf("the offending line went missing: %q", found[0].Message) + } +} + +// clip is the one place that escaping happens, so the two edges it owns are +// pinned here: a cut on a rune boundary rather than a byte one, and text that was +// already clean coming back the way it went in. +func TestClipCutsRunesAndLeavesCleanTextAlone(t *testing.T) { + if got := clip(" feat: a line "); got != "feat: a line" { + t.Errorf("clip collapsed to %q", got) + } + long := clip(strings.Repeat("é", 200)) + if !utf8.ValidString(long) { + t.Errorf("clip cut a rune in half: %q", long) + } + if n := utf8.RuneCountInString(long); n != 64 { + t.Errorf("clip returned %d runes, want 64", n) + } +} From fa8afdddf2f98d23e58bef41b536d2fe93f5aaab Mon Sep 17 00:00:00 2001 From: prode Date: Thu, 17 Sep 2026 10:46:55 -0300 Subject: [PATCH 2/2] fix(attribution): quoting a shape is mentioning it, not wearing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The package comment names this failure at the top of the file: a pattern that could match mid-sentence fires on a commit message that merely quotes one of these lines, which is exactly what a fix for this finding looks like. The other rules are anchored against it — a trailer at the start of a line, a badge or a link or a mention alone on one. The footer family had nothing, and the instrumental voice added in the previous commit had nothing either. Measured on this branch's own record. The commit explaining the widened rule quotes the phrasings it now catches, and the pull request body quotes them again, in a list. Both were reported by the rule they were explaining, and `scc validate --pr` came back exit 2 on the change that had just been pushed. So an occurrence behind an odd number of straight quotes or backticks, or inside a typographic quotation still open, is skipped — per occurrence rather than per line, so an unquoted credit beside a quotation is still caught. The apostrophe is deliberately not counted: it is a possessive far more often than it opens anything, and the instrumental rule already reads one three words along to tell a credit from a description. What this gives up is a footer written inside quotation marks, which is a shape nothing writes and a person would read as a mention anyway. What it buys is the one property this package trades everything for: one wrong finding teaches the reader to disbelieve the other ten. Verified against this repository's whole record — 149 files, every pull request body, issue comment and review comment, plus this branch's own commit and the body of #52: 54 hits, every one a real footer or session link from the pull requests that predate the rule, and nothing else. --- internal/attribution/attribution.go | 65 ++++++++++++++++++------ internal/attribution/attribution_test.go | 23 +++++++++ 2 files changed, 72 insertions(+), 16 deletions(-) diff --git a/internal/attribution/attribution.go b/internal/attribution/attribution.go index 229ad0a..182951a 100644 --- a/internal/attribution/attribution.go +++ b/internal/attribution/attribution.go @@ -230,14 +230,7 @@ func scanLine(line string, last bool) (Hit, bool) { if m := trailer.FindStringSubmatch(line); m != nil && Names(m[2]) { return Hit{Rule: RuleTrailer, Match: strings.TrimSpace(line)}, true } - // The name has to come *after* the phrase, not merely somewhere on the line. - // Measured on this project's own history: "opencode has one AGENTS.md written - // by whichever ran first" carries the phrase and the vocabulary and is a - // sentence about opencode, not a line signed by it. - if at := footer.FindStringIndex(line); at != nil && Names(line[at[1]:]) { - return Hit{Rule: RuleFooter, Match: strings.TrimSpace(line)}, true - } - if instrumental(line) { + if credited(line) || instrumental(line) { return Hit{Rule: RuleFooter, Match: strings.TrimSpace(line)}, true } if m := badge.FindStringSubmatch(line); m != nil && (Names(m[1]) || Names(m[2])) && signsAlone(line, m[0]) { @@ -252,6 +245,26 @@ func scanLine(line string, last bool) (Hit, bool) { return Hit{}, false } +// credited reports a line that hands this change to whoever it names: the footer +// phrase, or one of the ways somebody rephrases it. +// +// The name has to come *after* the phrase, not merely somewhere on the line. +// Measured on this project's own history: "opencode has one AGENTS.md written by +// whichever ran first" carries the phrase and the vocabulary and is a sentence +// about opencode, not a line signed by it. +// +// And the occurrence must not be quoted, for the reason instrumental gives below: +// a commit explaining this rule quotes the phrases it catches, and the rule that +// reported that would fire on the fix for its own finding. +func credited(line string) bool { + for _, at := range footer.FindAllStringIndex(line, -1) { + if Names(line[at[1]:]) && !quoted(line[:at[0]]) { + return true + } + } + return false +} + // instrumental reports a line that uses an assistant to do this work — "used // Claude Code to draft the tests", "ran Codex over the diff". // @@ -268,16 +281,36 @@ func scanLine(line string, last bool) (Hit, bool) { // about a hook surface; "using Claude Code to write this" hands over the work. // Those two tests are cheap, and they stand in for a distinction that would // otherwise need a parser. +// A quoted occurrence is skipped, which is the anchoring the other rules get from +// their shape and this one has nowhere else to get. Measured on this package's own +// history: a commit explaining this very rule quotes the line it now catches, and +// a check that reported that would fire on the fix for its own finding — the +// failure the package comment names at the top of this file. func instrumental(line string) bool { - at := useVerb.FindStringIndex(line) - if at == nil { - return false - } - rest := strings.Fields(line[at[1]:]) - if len(rest) == 0 || !Names(rest[0]) { - return false + for _, at := range useVerb.FindAllStringIndex(line, -1) { + rest := strings.Fields(line[at[1]:]) + switch { + case len(rest) == 0, !Names(rest[0]): + case strings.ContainsAny(firstWords(rest, 3), "'’"): + case quoted(line[:at[0]]): + default: + return true + } } - return !strings.ContainsAny(firstWords(rest, 3), "'’") + return false +} + +// quoted reports whether what follows this much of a line sits inside a +// quotation: an odd number of straight quotes or backticks behind it, or a +// typographic quotation still open. +// +// The apostrophe is deliberately not counted. It is a possessive in ordinary +// English far more often than it opens anything, and instrumental already reads +// one three words along to tell a credit from a description. +func quoted(before string) bool { + return strings.Count(before, `"`)%2 == 1 || + strings.Count(before, "`")%2 == 1 || + strings.Count(before, "“") > strings.Count(before, "”") } // firstWords is the opening of a fragment, punctuation kept. Kept rather than diff --git a/internal/attribution/attribution_test.go b/internal/attribution/attribution_test.go index 39ba834..e8a0328 100644 --- a/internal/attribution/attribution_test.go +++ b/internal/attribution/attribution_test.go @@ -359,3 +359,26 @@ func TestAFileIsNotAName(t *testing.T) { } } } + +// TestQuotingTheShapeIsNotWearingIt is the failure the package comment names at +// the top of the file: a commit message that merely quotes one of these lines, +// which is exactly what a fix for this finding looks like. Measured on this +// change's own commit and pull request, both of which were reported by the rule +// they were explaining. +func TestQuotingTheShapeIsNotWearingIt(t *testing.T) { + for _, line := range []string{ + `voice, "used Claude Code to draft the tests". The last needs the stricter test`, + "the instrumental voice, `used Claude Code to draft the tests`, needs a stricter test", + `"thanks to", "with the help of", and "used Claude Code to" are the phrasings it catches`, + } { + if hits := Scan("fix: thing\n\n" + line); len(hits) != 0 { + t.Errorf("a quoted shape reported as a signature: %q → %+v", line, hits) + } + } + // And an unquoted credit on the same line as a quotation is still caught, so + // the gate is per occurrence rather than per line. + line := `The footer said "Generated with X", and I used Claude Code to rewrite it.` + if hits := Scan("fix: thing\n\n" + line); len(hits) != 1 { + t.Errorf("the unquoted credit went unreported: %q → %+v", line, hits) + } +}