From 3ca8adc774f5e822fa203e219c8c72a2676ca693 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Tue, 27 Oct 2026 12:08:19 -0400 Subject: [PATCH 1/2] fix(ownership-scan): a bare token is ownership in a declaration, not in prose MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Repository branch binding` ended its alternation with the bare token `Branch` and `Repository revision binding` with the bare `Revision`; the scan greps case-insensitively, so either English word in any comment in a scannable file failed the guard. Seen on codefly-dev/service-nextjs#186, where the one finding was FORBIDDEN [Repository branch binding] runtime.go:1221: // Resolved before the container branch: a lock ... about an `if` below that line. A gate that reports a word rather than an ownership concept teaches the next author to reword a comment until it goes quiet, which is the move this guard exists to prevent. Both bare tokens now have to appear in a declaration: a manifest key or list entry, an assignment, a field declaration, or a struct tag. The explicit spellings (repoBranch, gitBranch, targetRevision, repository-revision, …) and refs/heads/ stay matched wherever they appear, and a selector read stays allowed — as `.Revision` already was, and now `.Branch` too, consistently. Also repairs a dead assertion found beside it, under the scout rule. The contract-field case read `if out="bash: : No such file or directory"` — a literal assignment, always true — so the suite reported "reading a released contract revision field is not ownership" without ever running the scan: a gate test green about a name rather than about the gate. Restored, it passes for the right reason, and the fixture it protects is now actually scanned. Co-Authored-By: Claude Opus 5 --- actions/manifest-ownership-scan/scan.sh | 12 +++++-- actions/manifest-ownership-scan/scan_test.sh | 35 +++++++++++++++++-- .../testdata/runtime-prose/runtime.go | 14 ++++++++ .../token-declaration-forms/runtime.go | 10 ++++++ .../token-declaration-forms/source.yaml | 6 ++++ 5 files changed, 72 insertions(+), 5 deletions(-) create mode 100644 actions/manifest-ownership-scan/testdata/runtime-prose/runtime.go create mode 100644 actions/manifest-ownership-scan/testdata/token-declaration-forms/runtime.go create mode 100644 actions/manifest-ownership-scan/testdata/token-declaration-forms/source.yaml diff --git a/actions/manifest-ownership-scan/scan.sh b/actions/manifest-ownership-scan/scan.sh index 40ae6d7..186ee1e 100755 --- a/actions/manifest-ownership-scan/scan.sh +++ b/actions/manifest-ownership-scan/scan.sh @@ -58,8 +58,16 @@ PATTERNS=( "Argo Application::kind[\"']?[[:space:]]*:[[:space:]]*[\"']?Application([\"']|[^[:alnum:]_]|$)|(type|class|struct|interface)[[:space:]]+Application([^[:alnum:]_]|$)" "Argo AppProject::kind[\"']?[[:space:]]*:[[:space:]]*[\"']?AppProject([\"']|[^[:alnum:]_]|$)|(type|class|struct|interface)[[:space:]]+AppProject([^[:alnum:]_]|$)" 'Repository URL binding::(^|[^[:alnum:]_])(repoURL|repositoryURL|repository_url|repository-url)([^[:alnum:]_]|$)' - 'Repository branch binding::(^|[^[:alnum:]_])(repoBranch|repositoryBranch|repository_branch|repository-branch|gitBranch|git_branch|git-branch|Branch)([^[:alnum:]_]|$)|refs/heads/' - 'Repository revision binding::(^|[^[:alnum:]_])(targetRevision|repoRevision|repositoryRevision|repository_revision|repository-revision|gitRevision|git_revision|git-revision)([^[:alnum:]_]|$)|(^|[^[:alnum:]_.])Revision([^[:alnum:]_]|$)' + # The bare token `Branch` is ownership in a DECLARATION — a manifest key, an + # assignment, a field, a struct tag — not in prose. Matching it anywhere made + # the English word in a comment explaining control flow read as repository + # binding, which is a finding about a word rather than about ownership. The + # explicit spellings and refs/heads/ stay matched wherever they appear. + "Repository branch binding::(^|[^[:alnum:]_])(repoBranch|repositoryBranch|repository_branch|repository-branch|gitBranch|git_branch|git-branch)([^[:alnum:]_]|$)|refs/heads/|^[[:space:]]*(-[[:space:]]*)?[\"']?branch[\"']?[[:space:]]*[:=]|^[[:space:]]*branch[[:space:]]+[^[:space:]]|[\"']branch[\"']" + # Same rule as Branch above: a bare token is ownership in a declaration, not + # in prose. `.Revision` was already allowed as a selector read; the English + # word in a comment now is too. + "Repository revision binding::(^|[^[:alnum:]_])(targetRevision|repoRevision|repositoryRevision|repository_revision|repository-revision|gitRevision|git_revision|git-revision)([^[:alnum:]_]|$)|^[[:space:]]*(-[[:space:]]*)?[\"']?revision[\"']?[[:space:]]*[:=]|^[[:space:]]*revision[[:space:]]+[^[:space:]]|[\"']revision[\"']" 'Git library::github\.com/(go-git/go-git|libgit2/git2go|src-d/go-git)/|org\.eclipse\.jgit|git2::Repository|simple-git|isomorphic-git|nodegit|GitPython|dulwich' 'GitHub API client::github\.com/google/go-github/|github\.com/shurcooL/githubv4|github\.NewClient|@octokit/|PyGithub|githubkit|octocrab' 'Pull request operation::PullRequest|pull_request|pull-request|pulls\.(Create|Edit|List|Merge)' diff --git a/actions/manifest-ownership-scan/scan_test.sh b/actions/manifest-ownership-scan/scan_test.sh index ccf19a5..3390854 100755 --- a/actions/manifest-ownership-scan/scan_test.sh +++ b/actions/manifest-ownership-scan/scan_test.sh @@ -68,11 +68,40 @@ fi # Reading a released RPC request field named Revision is not ownership: the # plugin declares neither the type nor a repository binding. The declaration # form stays a violation, as the runtime-identifiers fixture above asserts. -if out="bash: : No such file or directory"; then +if out="$(SCAN_ROOT="$here/testdata/contract-field" bash "$scan" 2>&1)"; then pass "reading a released contract revision field is not ownership" else - fail "contract field read should pass but failed:"$' -'"" + fail "contract field read should pass but failed:"$'\n'"$out" +fi + +# Prose is not ownership. The English words "branch" and "revision" in a +# comment that explains control flow name no repository, and a gate that +# reports one answers about a word rather than about ownership. Every +# declaration form still fails, below. +if out="$(SCAN_ROOT="$here/testdata/runtime-prose" bash "$scan" 2>&1)"; then + pass "a comment explaining control flow is not branch or revision binding" +else + fail "prose comment should pass but failed:"$'\n'"$out" +fi + +# A manifest key, a list entry, a field, a struct tag and an assignment are all +# declarations of a branch or revision binding and stay violations. +if out="$(SCAN_ROOT="$here/testdata/token-declaration-forms" bash "$scan" 2>&1)"; then + fail "token declaration forms should fail but passed:"$'\n'"$out" +else + for expectation in \ + "[Repository branch binding] runtime.go:4:" \ + "[Repository branch binding] runtime.go:9:" \ + "[Repository branch binding] source.yaml:3:" \ + "[Repository branch binding] source.yaml:6:" \ + "[Repository revision binding] runtime.go:5:" \ + "[Repository revision binding] source.yaml:4:"; do + if grep -qF "$expectation" <<<"$out"; then + pass "token declaration reported: $expectation" + else + fail "token declaration scan missed: $expectation"$'\n'"$out" + fi + done fi # Excluded paths must not be the reason a dirty tree fails: prove the release diff --git a/actions/manifest-ownership-scan/testdata/runtime-prose/runtime.go b/actions/manifest-ownership-scan/testdata/runtime-prose/runtime.go new file mode 100644 index 0000000..b729416 --- /dev/null +++ b/actions/manifest-ownership-scan/testdata/runtime-prose/runtime.go @@ -0,0 +1,14 @@ +package runtime + +// A comment that explains control flow is not ownership. This function takes +// the container branch when it must install nothing, whatever revision of the +// plan reached it, and no repository is bound anywhere in it. The declaration +// forms stay violations — see the runtime-identifiers and +// token-declaration-forms fixtures. +func installsLinkedPackages(container bool) bool { + if container { + // Resolved before the container branch below, at any revision. + return false + } + return true +} diff --git a/actions/manifest-ownership-scan/testdata/token-declaration-forms/runtime.go b/actions/manifest-ownership-scan/testdata/token-declaration-forms/runtime.go new file mode 100644 index 0000000..0a6a360 --- /dev/null +++ b/actions/manifest-ownership-scan/testdata/token-declaration-forms/runtime.go @@ -0,0 +1,10 @@ +package runtime + +type Source struct { + Branch string `json:"branch"` + Revision string `json:"revision"` +} + +func bind(s *Source) { + Branch = s.Branch +} diff --git a/actions/manifest-ownership-scan/testdata/token-declaration-forms/source.yaml b/actions/manifest-ownership-scan/testdata/token-declaration-forms/source.yaml new file mode 100644 index 0000000..c3644d5 --- /dev/null +++ b/actions/manifest-ownership-scan/testdata/token-declaration-forms/source.yaml @@ -0,0 +1,6 @@ +spec: + source: + branch: main + revision: HEAD + tags: + - branch: release From c2306ffaab5df83dd883b33af1911c91cd83cefd Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Tue, 27 Oct 2026 12:53:51 -0400 Subject: [PATCH 2/2] fix(ownership-scan): bind on the syntax, not on a declaration at line start MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the first commit found it wrong in both directions, and the second one matters more than the first. A comment that QUOTED either word still red: the quoted-key alternative was a bare `"branch"` with no colon, so // The caller names the "branch" it took, and the "revision" it saw. produced FORBIDDEN for both tokens — the very class this change exists to remove. The prose fixture passed only because it never quoted either word. And requiring a declaration at line start caught far less than the first commit's message claimed. All of these scanned CLEAN: cfg.Branch = "main" cfg.Revision = "HEAD" cfg.SetBranch(s.Branch) Source{Branch: "main", Revision: "HEAD"} Only a bare identifier at line start was caught — `Branch = s.Branch`, which nobody writes in Go. So `cfg.Branch = branchFromManifest()`, a real repository binding, passed a guard that main's bare-token pattern would have caught: the change narrowed the gate in one direction and weakened it in the other. The rule is now that a bare token is ownership where the SYNTAX binds it — a manifest key or list entry, a quoted key (with the colon that makes it one), a struct tag, a field declaration, a selector assignment, a struct-literal field, a setter or builder call, a bare assignment. A plain read stays allowed: `[.>]token[[:space:]]*=[^=]` does not match `req.Revision == ""`. Fixtures pin both directions, so the suite fails without this: the prose fixture now quotes both words, and token-declaration-forms carries every binding spelling. Against the first commit's patterns the suite reports 6 failures — one prose false positive and five missed bindings. Co-Authored-By: Claude Opus 5 --- actions/manifest-ownership-scan/scan.sh | 22 +++++++++-------- actions/manifest-ownership-scan/scan_test.sh | 24 +++++++++++++------ .../testdata/runtime-prose/runtime.go | 7 +++--- .../token-declaration-forms/runtime.go | 9 ++++++- 4 files changed, 41 insertions(+), 21 deletions(-) diff --git a/actions/manifest-ownership-scan/scan.sh b/actions/manifest-ownership-scan/scan.sh index 186ee1e..a05e41d 100755 --- a/actions/manifest-ownership-scan/scan.sh +++ b/actions/manifest-ownership-scan/scan.sh @@ -58,16 +58,18 @@ PATTERNS=( "Argo Application::kind[\"']?[[:space:]]*:[[:space:]]*[\"']?Application([\"']|[^[:alnum:]_]|$)|(type|class|struct|interface)[[:space:]]+Application([^[:alnum:]_]|$)" "Argo AppProject::kind[\"']?[[:space:]]*:[[:space:]]*[\"']?AppProject([\"']|[^[:alnum:]_]|$)|(type|class|struct|interface)[[:space:]]+AppProject([^[:alnum:]_]|$)" 'Repository URL binding::(^|[^[:alnum:]_])(repoURL|repositoryURL|repository_url|repository-url)([^[:alnum:]_]|$)' - # The bare token `Branch` is ownership in a DECLARATION — a manifest key, an - # assignment, a field, a struct tag — not in prose. Matching it anywhere made - # the English word in a comment explaining control flow read as repository - # binding, which is a finding about a word rather than about ownership. The - # explicit spellings and refs/heads/ stay matched wherever they appear. - "Repository branch binding::(^|[^[:alnum:]_])(repoBranch|repositoryBranch|repository_branch|repository-branch|gitBranch|git_branch|git-branch)([^[:alnum:]_]|$)|refs/heads/|^[[:space:]]*(-[[:space:]]*)?[\"']?branch[\"']?[[:space:]]*[:=]|^[[:space:]]*branch[[:space:]]+[^[:space:]]|[\"']branch[\"']" - # Same rule as Branch above: a bare token is ownership in a declaration, not - # in prose. `.Revision` was already allowed as a selector read; the English - # word in a comment now is too. - "Repository revision binding::(^|[^[:alnum:]_])(targetRevision|repoRevision|repositoryRevision|repository_revision|repository-revision|gitRevision|git_revision|git-revision)([^[:alnum:]_]|$)|^[[:space:]]*(-[[:space:]]*)?[\"']?revision[\"']?[[:space:]]*[:=]|^[[:space:]]*revision[[:space:]]+[^[:space:]]|[\"']revision[\"']" + # A bare token is ownership where the syntax BINDS it: a manifest key or list + # entry, an assignment (`cfg.Branch = ...`), a field declaration, a struct tag, + # a struct-literal field, a setter call. Matching the word anywhere made an + # English comment read as repository binding — a finding about a word rather + # than about ownership — while matching only a line-start identifier would let + # `cfg.Branch = branchFromManifest()` sail through the guard that exists to + # catch it. A plain READ (`req.Revision == ""`) stays allowed: reading a + # released contract field owns neither the type nor a binding. The explicit + # spellings and refs/heads/ stay matched wherever they appear. + "Repository branch binding::(^|[^[:alnum:]_])(repoBranch|repositoryBranch|repository_branch|repository-branch|gitBranch|git_branch|git-branch)([^[:alnum:]_]|$)|refs/heads/|^[[:space:]]*(-[[:space:]]*)?[\"']?branch[\"']?[[:space:]]*(:|=[^=])|^[[:space:]]*branch[[:space:]]+[^[:space:]]|[\"']branch[\"'][[:space:]]*:|:[\"']branch[\"']|[.>]branch[[:space:]]*=[^=]|[{,][[:space:]]*branch[[:space:]]*:|(set|with)_?branch[[:space:]]*\\(" + # Same rule as Branch above, including the allowed selector read. + "Repository revision binding::(^|[^[:alnum:]_])(targetRevision|repoRevision|repositoryRevision|repository_revision|repository-revision|gitRevision|git_revision|git-revision)([^[:alnum:]_]|$)|^[[:space:]]*(-[[:space:]]*)?[\"']?revision[\"']?[[:space:]]*(:|=[^=])|^[[:space:]]*revision[[:space:]]+[^[:space:]]|[\"']revision[\"'][[:space:]]*:|:[\"']revision[\"']|[.>]revision[[:space:]]*=[^=]|[{,][[:space:]]*revision[[:space:]]*:|(set|with)_?revision[[:space:]]*\\(" 'Git library::github\.com/(go-git/go-git|libgit2/git2go|src-d/go-git)/|org\.eclipse\.jgit|git2::Repository|simple-git|isomorphic-git|nodegit|GitPython|dulwich' 'GitHub API client::github\.com/google/go-github/|github\.com/shurcooL/githubv4|github\.NewClient|@octokit/|PyGithub|githubkit|octocrab' 'Pull request operation::PullRequest|pull_request|pull-request|pulls\.(Create|Edit|List|Merge)' diff --git a/actions/manifest-ownership-scan/scan_test.sh b/actions/manifest-ownership-scan/scan_test.sh index 3390854..8c1cd69 100755 --- a/actions/manifest-ownership-scan/scan_test.sh +++ b/actions/manifest-ownership-scan/scan_test.sh @@ -75,26 +75,36 @@ else fi # Prose is not ownership. The English words "branch" and "revision" in a -# comment that explains control flow name no repository, and a gate that -# reports one answers about a word rather than about ownership. Every -# declaration form still fails, below. +# comment that explains control flow name no repository — including when the +# comment QUOTES them, which a quoted-key pattern reds unless it requires the +# colon that makes a key a key. A gate that reports either answers about a word +# rather than about ownership. Every binding spelling still fails, below. if out="$(SCAN_ROOT="$here/testdata/runtime-prose" bash "$scan" 2>&1)"; then - pass "a comment explaining control flow is not branch or revision binding" + pass "a comment explaining control flow, quoting either word, is not binding" else fail "prose comment should pass but failed:"$'\n'"$out" fi -# A manifest key, a list entry, a field, a struct tag and an assignment are all -# declarations of a branch or revision binding and stay violations. +# Every spelling that BINDS a branch or revision stays a violation: a manifest +# key, a list entry, a field declaration, a struct tag, a selector assignment +# (`cfg.Branch = "main"`), a setter call (`cfg.SetBranch(...)`), a +# struct-literal field (`Source{Branch: ...}`) and a bare assignment. Catching +# only the last of those — the one nobody writes in Go — would leave the guard +# green on every real binding. if out="$(SCAN_ROOT="$here/testdata/token-declaration-forms" bash "$scan" 2>&1)"; then fail "token declaration forms should fail but passed:"$'\n'"$out" else for expectation in \ "[Repository branch binding] runtime.go:4:" \ - "[Repository branch binding] runtime.go:9:" \ + "[Repository branch binding] runtime.go:12:" \ + "[Repository branch binding] runtime.go:14:" \ + "[Repository branch binding] runtime.go:15:" \ + "[Repository branch binding] runtime.go:16:" \ "[Repository branch binding] source.yaml:3:" \ "[Repository branch binding] source.yaml:6:" \ "[Repository revision binding] runtime.go:5:" \ + "[Repository revision binding] runtime.go:13:" \ + "[Repository revision binding] runtime.go:15:" \ "[Repository revision binding] source.yaml:4:"; do if grep -qF "$expectation" <<<"$out"; then pass "token declaration reported: $expectation" diff --git a/actions/manifest-ownership-scan/testdata/runtime-prose/runtime.go b/actions/manifest-ownership-scan/testdata/runtime-prose/runtime.go index b729416..fff74bf 100644 --- a/actions/manifest-ownership-scan/testdata/runtime-prose/runtime.go +++ b/actions/manifest-ownership-scan/testdata/runtime-prose/runtime.go @@ -2,9 +2,10 @@ package runtime // A comment that explains control flow is not ownership. This function takes // the container branch when it must install nothing, whatever revision of the -// plan reached it, and no repository is bound anywhere in it. The declaration -// forms stay violations — see the runtime-identifiers and -// token-declaration-forms fixtures. +// plan reached it, and no repository is bound anywhere in it. The caller names +// the "branch" it took, and the "revision" it saw: quoting an English word +// does not bind one either. The declaration forms stay violations — see the +// runtime-identifiers and token-declaration-forms fixtures. func installsLinkedPackages(container bool) bool { if container { // Resolved before the container branch below, at any revision. diff --git a/actions/manifest-ownership-scan/testdata/token-declaration-forms/runtime.go b/actions/manifest-ownership-scan/testdata/token-declaration-forms/runtime.go index 0a6a360..6ffe361 100644 --- a/actions/manifest-ownership-scan/testdata/token-declaration-forms/runtime.go +++ b/actions/manifest-ownership-scan/testdata/token-declaration-forms/runtime.go @@ -5,6 +5,13 @@ type Source struct { Revision string `json:"revision"` } -func bind(s *Source) { +// Every spelling below BINDS a repository branch or revision. A guard that +// catches only a bare identifier at line start misses all but the last, which +// is the one nobody writes. +func bind(cfg *Config, s *Source) { + cfg.Branch = "main" + cfg.Revision = "HEAD" + cfg.SetBranch(s.Branch) + _ = Source{Branch: "main", Revision: "HEAD"} Branch = s.Branch }