Skip to content

fix(ownership-scan): a bare token is ownership in a declaration, not in prose - #10

Merged
antoinetoussaint-byte merged 2 commits into
mainfrom
fix/branch-binding-needs-declaration-shape
Oct 27, 2026
Merged

antoinetoussaint-byte merged 2 commits into
mainfrom
fix/branch-binding-needs-declaration-shape

Conversation

@antoinetoussaint-byte

@antoinetoussaint-byte antoinetoussaint-byte commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

What this is

A fix in the gate that owns the behaviour. manifest-ownership-scan reported an English
word rather than an ownership concept, so a comment explaining control flow failed the guard.

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 word, standing alone anywhere in a scannable file, was a violation. On
codefly-dev/service-nextjs#186 the single finding was:

FORBIDDEN [Repository branch binding] runtime.go:1221:  // Resolved before the container branch: a lock ...

That line is about an if below it. No repository is bound anywhere near it.

The rule, stated once for both tokens

A bare token is ownership where the syntax BINDS it. Matched: a manifest key or list entry,
a quoted key with the colon that makes it a key, a struct tag, a field declaration, a selector
assignment (cfg.Branch = "main"), a struct-literal field (Source{Branch: ...}), a setter or
builder call (cfg.SetBranch(...)), and a bare assignment. Unchanged:

  • the explicit spellings — repoBranch, repositoryBranch, repository-branch, gitBranch,
    targetRevision, repository_revision, gitRevision, … — still match wherever they
    appear, including in a comment, where they are a real smell;
  • refs/heads/ still matches anywhere;
  • a plain read stays allowed. [.>]token[[:space:]]*=[^=] does not match
    req.Revision == "": reading a released contract field owns neither the type nor a binding.
    .Revision already had this; .Branch now does too, which removes an inconsistency between
    the two patterns rather than introducing one.

The first version of this PR was wrong in both directions

Kept here because it is the same class the PR is correcting, and the second error is the worse
one. Review found:

  • 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 prose fixture passed only because it never
    quoted either word.
  • Requiring a declaration at line start caught far less than the body claimed.
    cfg.Branch = "main", cfg.Revision = "HEAD", cfg.SetBranch(s.Branch) and
    Source{Branch: "main", Revision: "HEAD"} all scanned clean. 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.

Both are fixed above, and both are now pinned by fixtures rather than by argument.

Evidence, both directions

From the action's own suite plus the case that triggered this:

prose fixture, now QUOTING both words           clean
selector read  (req.Revision == "")             clean
binding spellings, all reported —
  [branch]   struct tag, cfg.Branch = "main", cfg.SetBranch(...),
             Source{Branch: ...}, bare assignment, YAML key, YAML list entry
  [revision] struct tag, cfg.Revision = "HEAD", Source{... Revision: ...}, YAML key
runtime-identifiers (Branch/Revision fields)    still reported
dirty tree                                      all 13 labels still reported
scan_test.sh                                    all assertions passed

The suite fails without this fix. Against the first commit's patterns it reports 6
failures
— one prose false positive and five missed bindings — which is the check the first
version lacked.

The original trigger still holds: runtime.go at service-nextjs b2986f6, carrying the
original // Resolved before the container branch: comment that this guard rejected, scans
clean. So this addresses the real case, not a reworded version of it.

The service-nextjs reword is a dated stopgap. That repo's 0d82b4b renamed the comment to
"the container check below" purely to get past this scan, and said so. Once this lands, the
reword is unnecessary and the comment can say "branch" again.

Also here: a gate test that reported a verdict without ever running the gate

Found beside the pattern and fixed under the scout rule. The contract-field case read:

if out="bash: : No such file or directory"; then
  pass "reading a released contract revision field is not ownership"

That is a literal assignment, so the condition is always true. The suite printed a verdict about
the scan without ever invoking it, and the testdata/contract-field/ fixture had never been
scanned — the green-about-a-name class, inside the tests of a guard whose whole purpose is to
be trusted. Restored to run the scan, it passes for the right reason: req.Revision is a
selector read the pattern permits. Had it been live, it would also have caught this PR
regressing that case.

Reuse

No new mechanism: one pattern string per token, the existing PATTERNS loop, the existing
fixture-directory convention, and the existing scan_test.sh harness. Deliberately not written:
a comment-stripping pass over source files (a second parser for a job a shape anchor does), and
a path or inline exclusion (the action rejects those for runtime source on purpose, and a
per-repo opt-out would hide real bindings).

🤖 Generated with Claude Code

Fleet-wide trap found while doing this, worth stating here

The private-git wrapper these agents use pipes git through sed to mask tokens, so the
pipeline's exit status is sed's, not git's. git ls-remote --exit-code through it always
reports success, and a tag-existence check built on $? reported a tag that did not exist. Any
check through such a wrapper must test output, never $?. Same class as this PR's finding:
a check that is green about something other than what it claims to measure.

antoinetoussaint-byte and others added 2 commits October 27, 2026 12:09
…in prose

`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 <noreply@anthropic.com>
… start

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 <noreply@anthropic.com>
@antoinetoussaint-byte
antoinetoussaint-byte merged commit 5833261 into main Oct 27, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant