Skip to content

identity: #549's answer is the field table that already shipped, plus the door test it was missing - #585

Merged
vyskocilm merged 4 commits into
mainfrom
identity/checktext-through-resolve
Sep 18, 2026
Merged

vyskocilm merged 4 commits into
mainfrom
identity/checktext-through-resolve

Conversation

@vyskocilm

Copy link
Copy Markdown
Contributor

Answers #549's decision, and the answer is option 2, already shipped. Nothing here redesigns anything; it closes the one gap left over and corrects the design doc the ticket is about. Everything measured at 1487aec, not at the ticket's dedaf7a.

The decision

Option 2 — a structural field table — is the ruling, and #454 built it. internal/policy/identity.go:

var identityFields = mustIdentityFields(reflect.TypeOf(Identity{}), nil, "")

The type is the table. CheckText walks it, Resolve's expand-and-symlink loop walks it, mustIdentityFields panics at package init on a missing snug: tag, TestIdentityHasNoReferenceKindedField walks the type independently, and internal/profile/identitynested_test.go asserts the TOML decode structs agree leaf for leaf. The ticket's acceptance criterion —

A new identity field must not be expressible without passing through CheckText.

— is what that derivation buys, and the comment above it says so.

Empirical confirmation: #453 was the ticket's own test case. It called a signing key "the seventh field" and blocked it on this decision. It landed anyway, as git.signing_key (41192d0, #573), with no seam, no adapter registry, and no edit to the hand-maintained injection list the ticket calls site 2 — because that list stopped being hand-maintained in b994603 (#575).

Option 1 — a real adapter seam — is refused. Not on cost. On the set:

tool SECRETS.md §6 credential inside after
ssh already the agent proxy none, and none today
git §6.1 "done, and nothing to build" none, and none today
claude §6.2 broker none
gh §6.3 a smaller credential, "where none is available, gh has no credentials inside" smaller, or none

A seam abstracts "acquire on the host, filter through a whitelist, author a file inside". Three of the four adapters are slated to stop doing that. Building the abstraction now fixes the shape of a set that is not staying at four and is not staying this shape.

§4.1 already refuses the mechanism on its own grounds — "There is no adapter mechanism, and that is deliberate", with a bar in place of a registry, because an adapter runs on the HOST and a plugin layer loads from a config path XDG_CONFIG_HOME can repoint at a checked-out repository.

Option 3 — documentation only — is already met. §2 tabulates what is inside today with its code; §4.1 states the absence of a mechanism as a decision rather than a gap. That is the ticket's "documented as adhoc rather than presented as a design".

The ticket body is stale in four places

SECRETS.md is 1055 lines, not 2271. The Status: DRAFT. Nothing here is built. line it quotes at line 3 is gone. §5's D3, which option 1 was said to overturn, no longer exists — the pruning pass took §5 with it, so option 1 needs no overturning; §4.1 is what stands. And its blocker #453 is closed.

What this PR actually changes

1. The CheckText door (internal/policy/identitytext_test.go)

CheckText has exactly one non-test call site, in Resolve's profile loop. Two tests cover it and neither holds it for a leaf that does not exist yet:

  • TestCheckTextRefusesAForgingRuneAtEveryIdentityLeaf drives identityFields, so it grows with the struct — but calls id.CheckText directly and never reaches Resolve.
  • TestIdentityFieldsRefuseControlCharacters goes through Resolve with the real config-injection payloads — but its table is hand-written, ten rows over seven of the eight leaves.

So an eighth leaf is held by neither on the path a profile takes.

The new TestEveryIdentityLeafIsRefusedThroughResolve drives identityFields through Resolve and asserts the error carries CheckText's own wording. That last assertion is load-bearing, not decoration. Measured with the CheckText call deleted — all eight subtests fail, and three of them fail on the wording rather than on err == nil:

--- FAIL: .../ssh.host
    error did not carry CheckText's own wording, so this leaf was refused for some
    other reason: profile "pinned": identity.ssh.host = "‮" names a host and
    identity.gh.host does not ...
--- FAIL: .../ssh.agent
    ... profile "pinned": unknown identity.ssh.agent "proxy‮" (want proxy or none)
--- FAIL: .../git.name
    identity.git.name carrying a forging rune (U+202E) resolved through Resolve with no error

refuseHalfNamedHost and ParseSSHMode refuse ssh.host, gh.host and ssh.agent on their own, so a bare err != nil on those three keeps passing after the door is removed. Same measurement on the two existing tests: the direct-CheckText one passes entirely with the door gone; the hand table fails 7 of its 10 rows and passes 3, for the same reason — gh.host trips the host-pairing refusal, git.signing_key's payload leaves agent unset so it trips the signing_key needs agent = "proxy" refusal, and ssh.agent's "none\n" fails ParseSSHMode.

Both existing tests are unchanged. The hand table carries the real payloads (x\n[core]\n\tsshCommand = evil, the C1 and bidi rows) and is worth keeping as written.

2. SECRETS.md §2 cited two lines that had moved

Section 2 is the table a reader consults to answer which credential is inside and which code puts it there. Two of its three citations were false:

Neither edit touched the table, nothing failed, both read as precise. Fixed to symbol citations, which move with the code. internal/cli/secretstable_test.go holds both halves — the symbol a row names is in the file it names, and no row carries a file.go:NNN at all:

--- FAIL: SECRETS.md §2 cites `internal/policy/gitextract.go:131`. A line number is a
          copy of state held in the file it names ... Cite the symbol instead
--- FAIL: SECRETS.md §2 cites "GitConfigFromRenamed" in internal/policy/gitextract.go
          and that file does not contain it

Scope is that one table. The rest of .claude/design cites lines in hundreds of places and sweeping them is a change of its own.

Verification

make gate green. No production code changes — tests and a design document — so no golden argv diff and no red-team round: this touches no profile, no mount generation, no seccomp filter and no host-integration surface.

Closes #549.

🤖 Generated with Claude Code

vyskocilm and others added 3 commits September 17, 2026 21:26
…nd table

CheckText has exactly one non-test call site, internal/policy/resolve.go, in
Resolve's profile loop. Two tests already covered it and neither holds the door
for a leaf that does not exist yet:

  TestCheckTextRefusesAForgingRuneAtEveryIdentityLeaf drives identityFields, so
  it grows with the struct, but calls id.CheckText directly and says nothing
  about whether resolution reaches it.

  TestIdentityFieldsRefuseControlCharacters does go through Resolve, with the
  real config-injection payloads, but its table is hand written: seven rows for
  the eight leaves Identity has today.

So an eighth leaf added tomorrow is refused by neither on the path a profile
actually takes, which is the gap issue #549 names in one sentence: a new
identity field must not be expressible without passing through CheckText.

The new test drives identityFields through Resolve and asserts the refusal
carries CheckText's own wording. That last assertion is not decoration.
identity.ssh.host is independently refused by the host-pairing rule when
gh.host is unset, so with the door removed that leaf still fails to resolve --
for a reason that has nothing to do with a forging rune. Measured by deleting
the CheckText call: all eight subtests fail, and ssh.host fails on the wording
assertion rather than on err == nil.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Section 2 is the table a reader consults to answer which credential is inside
the sandbox and which code puts it there. Two of its three code citations were
false:

  internal/cli/identity.go:330 was offered as the oauth_token: write. That line
  is a pol.Replace inside the allowed_signers staging issue #453 inserted above
  it; the real write is :471, in stageGhConfig.

  internal/policy/gitextract.go:131 was offered as the insteadOf rewrite. That
  line is the id.Git.Email override; the real rewrite is :176, in
  GitConfigFrom.

Neither edit touched the table, nothing failed, and both citations read as
precise. A line number is a copy of state held in the file it names, so the fix
is to cite the symbol, which moves with the code.

The test holds both halves: the symbol a row names is really in the file it
names, and no row carries a file.go:NNN at all. Scope is this one table on
purpose -- the rest of .claude/design cites lines in hundreds of places, and
sweeping them is a change of its own.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
@vyskocilm
vyskocilm merged commit eded9ff into main Sep 18, 2026
5 checks passed
@vyskocilm
vyskocilm deleted the identity/checktext-through-resolve branch September 18, 2026 18:54
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.

Redesign the secrets model: four per-tool adapters and a struct that grows a field per tool

1 participant