chore: gitignore agent worktrees; document the flags that shipped unmentioned, and guard both - #105
Merged
Merged
Conversation
Agent worktrees land under .claude/worktrees/. Untracked, they stamped every local build "+dirty" -- which reaches the wire in the user-agent and the MCP gateway handshake's clientInfo.version -- and a `git add -A` would have committed a whole nested checkout into this public repo. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…the next one Five flags were reachable from --help but named in neither README.md nor cmd/agents.md: apps set-owners --wait/--wait-timeout, auth token --json, and mcp servers register --token-sharing/--source-app-id. A sixth, completion's --no-descriptions, surfaced once the guard initialized cobra's lazy subtree. The failure this causes is not a typo, it is an agent rebuilding by hand what c1i already does -- a pagination loop where --paginate would have done, an MCP registration where --tool-prefix would have. cmd/agents.md also gained a "Global flags" section: the agent-facing index named only three of the six persistent flags, leaving --debug, --max-retries and --error-format discoverable from the README alone. It now also points at --list-key and --allow-delete-body beside the `c1i api` conventions they apply to, and at --wait beside the async owner-provisioning warning. TestEveryFlagIsDocumented fails CI on any long flag absent from both docs; TestGlobalFlagsDocumentedInAgentsDoc holds the persistent flags to both. Matching is boundary-anchored, so --wait is not satisfied by a doc that only mentions --wait-timeout. The guard forces cobra's lazily-created completion subtree and --version/--help flags so its inventory does not depend on whether an earlier test in the package happened to run a command. The exemption map is empty. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both commands shipped without ever appearing in the README's Apps section, so --help was the only way to find them. Documented the way their neighbours are: one usage line each in the section's code block (create after get, delete last, matching the Policies section's ordering) plus a short paragraph covering what --help alone doesn't make obvious -- that "apps create" auto-assigns the caller as an owner and returns the app under an "app" key that --fields never projects, and that "apps delete" is a soft-delete whose endpoint is live but absent from the OpenAPI spec, so "docs endpoints" never lists it (verified against the shipped spec, not just the command's own help text). Also reframes the flag guard's per-name scope note: coverage is per flag name rather than per (command, flag) pair by decision, not by omission, so the comment now says the alternative was weighed and rejected and why. A reader should not mistake it for unfinished work and "upgrade" it into a per-command flag matrix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review found this branch doing the exact thing it exists to prevent: shipping
text the code contradicts. Each fix below was verified by running the command
or reading the file, not by trusting a plausible source.
cmd/agents.md said `mcp gateway` goes through a separate HTTP client honoring
neither --debug nor --max-retries. cmd/mcp_gateway.go:77-91 threads both,
deliberately. The cost was concrete: an agent debugging a hanging gateway call
would read the highest-traffic agent doc in the repo and never try the one flag
that shows where it stopped. The same section also claimed the empty-path and
redirect guards were REST-only; mcpgateway.New builds on internal/transport,
whose New wraps redirectTripper and whose Do checks pathHasEmptySegment
unconditionally. Both corrected.
CLAUDE.md's "both are currently silently inert on the packages that issue their
own HTTP" is where that false sentence came from, so it is corrected in the
same pass -- otherwise the next author writes it again. The part of the rule
that still applies (build on internal/transport; don't hand-roll http.Client)
is kept.
apps delete's endpoint IS in the OpenAPI spec: DELETE /api/v1/apps/{id},
operation c1.api.app.v1.Apps.Delete, and `docs endpoints` lists it. My earlier
check grepped for {app_id} when the spec uses {id} -- the measurement failed,
not the fact. Removed from the README and from the shipped help at
cmd/apps_delete.go, where it has sat since c9755bb. Also trimmed "drops out of
normal listings; retained for audit", which nothing in-repo supports, down to
what deleted_at on apps list rows actually shows.
CHANGELOG said five flags; the guard fails on six against main's docs.
--no-descriptions was documented and then left out of its own count.
CLAUDE.md's flag-doc bullet named only flagDocExemptions, but the global test
reads globalFlagDocExemptions -- an author following it would have used the
wrong map and found the escape hatch apparently broken.
Three claims trimmed to their evidence: --token-sharing's PER_USER
compatibility now mirrors the register help's full list (oauth2
authorization-code/passthrough, bearerToken, customHeader, basicAuth) instead
of passthrough alone, and carries the resolved-mode caveat so a normal
round-trip is not read as a bug; the access token's "audience-scoped" claim is
dropped, since the only aud in the tree is on the client-assertion JWT sent to
the token endpoint and the returned token is never inspected; and `git add -A`
on a worktree records a gitlink (mode 160000, reproduced), not a whole nested
checkout.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rim a comment The "Reject an empty id" bullet still said a package issuing its own HTTP inherits neither the empty-path nor the redirect guard. False for internal/mcpgateway and internal/login -- both get them from transport.New -- and it contradicted the bullet added five lines below in the previous commit. Worse, it is the bullet a new-subsystem author reads first, so it would have sent them off to hand-roll duplicates of guards they already had: the exact failure this branch exists to close, reproduced inside the rulebook. Restructured the three bullets so the shared fact is stated once: build on internal/transport and you inherit both guards plus --debug/--max-retries; the warnings that follow now scope to hand-rolling http.Client, which is what the original rule was actually about. Also narrowed the claim that "auth login" honors both flags -- PollForToken appends WithMaxRetries(0) after the caller's opts, deliberately, since the RFC 8628 poll interval is that call's retry strategy. auth token's help still promised the token is "audience-scoped to the C1 API host". The token request sends client_id, grant_type, client_assertion_type and client_assertion only -- no audience, no resource -- and nothing decodes the returned token, so the CLI neither requests nor observes it. Dropped, matching how the same claim was handled for apps delete rather than fixing only the README. The instance in the 0.4.0 changelog section is left alone as released history; the Unreleased entry records the correction. Trimmed the flag guard's scope comment back toward its original length. The non-obvious fact is that coverage is per flag name rather than per (command, flag) pair; the argument for why the alternative was rejected was review history, which CLAUDE.md's own comment rule says not to narrate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ses it Three false claims, all introduced by the previous commit's restructure, all about the transport it was correcting claims about. transport.New does NOT thread --debug/--max-retries. It defaults to DefaultMaxRetries and debug-off and reads nothing ambient; both flags reach the wire only because every caller passes them by hand (internal/client from cfg; mcp_gateway, auth_login, auth_token from viper). Saying they come free would have sent a new-subsystem author to transport.New(base) with no options and shipped tracing that shows nothing -- the exact RCA this section exists for, re-committed by following the doc. Split into two bullets so the guards (which are free and unconditional) and the flags (which are not) can't be conflated again, and dropped "what makes the rest of this list free", which claimed credit for the paginate/typed-error/id-escaping bullets above it. "Every package that issues its own HTTP does this today" was false: three live sites bypass internal/transport entirely -- docs_search.go:51, :119 and docs_openapi.go:187, all http.DefaultClient.Do. Scoped the claim to internal/ packages and named the docs sites as the known exception, including that they return bare fmt.Errorf rather than a classifiable error. Not fixing that code here; it is being filed separately. cmd/agents.md carried the same fact in text this branch added: "These flags work on every command" and --debug traces "every HTTP request". Both inert on the five fetching docs subcommands, so an agent debugging an empty docs search would run --debug, see nothing, and conclude no request was sent -- the same misdiagnosis this branch just fixed for mcp gateway. Carved the exception out explicitly and said what silence there does and does not mean. Verified by running it, not by reading: with the OpenAPI cache moved aside, `docs endpoints --debug` fetched 940KB (cache rewritten) and printed zero stderr lines. The first attempt at that measurement read a warm cache and made no request at all, proving nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…h time --debug and --max-retries are inert on the five fetching docs subcommands, which call http.DefaultClient directly instead of internal/transport. That fact had reached six documents; the previous rounds fixed the two smallest copies. Fixed the four remaining unqualified claims. README carried the strongest form -- "everywhere the CLI sends HTTP" -- and by round 4 it contradicted the agents.md paragraph shipped in the same branch; also README's "applies to any command" for --max-retries and "traces each HTTP request" for --debug. .claude/commands/c1i.md's flag bullets made unqualified effect claims 30 lines above its own list of the five commands (its heading, which describes registration rather than effect, is left alone). CLAUDE.md's Global-flags summary delegated the whole question to README. Corrected a host claim I got wrong last round: these five do not all hit "the public docs site". docs openapi/endpoints/endpoint fetch conductorone.com, cached 24h -- which is why a run can return rows without sending a request at all, the thing that made my first measurement of this useless -- while docs search and docs page call a third party, api.mintlify.com, with a public client-side key. That distinction matters for egress rules and for why one can fail while the other works. The guard is the point. TestFlagScopeExceptionDocumented fails when any of the four documents names either flag without carving out the exception. TestDocumentedFlagScopeExceptionIsStillReal ties the carve-out to the code and fails in BOTH directions: a new cmd/ file bypassing the shared transport, or these files ceasing to bypass it, which would make every carve-out stale. So the fix that is being filed separately cannot silently leave six documents warning about a hazard that no longer exists. Proved load-bearing four ways: deleting the README carve-out and the c1i.md carve-out each failed naming that file; adding a new file using http.DefaultClient failed naming it; and making docs_search.go stop using it failed telling the author the carve-outs are now stale. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The scope guard matched the literal string "http.DefaultClient". Two escapes
followed from that, and the second is a defect rather than a gap.
A cmd/ file using &http.Client{} and http.Get passed, though the hazard is
identical and http.Get is the likelier accidental spelling, being DefaultClient
underneath. Worse: converting docs_search.go to a package-level &http.Client{}
-- still bypassing the transport, both flags still inert -- made the guard fail
with "no longer uses http.DefaultClient ... the docs carve-outs naming it are
stale", instructing the reader to delete carve-outs that were still true. A
guard that misdirects on its novel property is worse than one that says
nothing.
Inverted it to the property instead of taking the union of spellings: parse
each file's AST, resolve the net/http import's local name, and flag the
package-level senders (DefaultClient, Get, Post, Head, PostForm) and
http.Client composite literals. On the current tree that finds exactly the two
docs files plus internal/transport itself, which is allowlisted with a reason
-- it is the client every other package is supposed to inherit from. The
net/http-only limitation is stated in the comment: a hand-rolled RoundTripper
or a third-party HTTP library would still slip past.
The walk also covered cmd/ only, and non-recursively, so a bypass under
internal/ passed -- the likelier home for the next one, since the CLAUDE.md
section this backs is about internal/ packages. Now walks the repo from ..,
skipping .git/dev/vendor, with the anti-vacuity floor raised to 100 files.
Re-proved on the new detector: &http.Client{}+http.Get in cmd/ fails naming the
file; http.DefaultClient under internal/ fails naming it; converting a
bypassing file to &http.Client{} now correctly stays silent; and consolidating
docs_openapi.go onto a shared helper still fails with the stale-carve-out
message.
Prose: cmd/client.go is the single viper read every REST command inherits via
newClient, not something each one copies -- the old wording invited an author
to add a duplicate read. "those three sites" now reads "those three call
sites", since the clause before it enumerates five subcommands. agents.md's
"Nor do they all call the same place" meant the subcommands, not the flags.
Rewrapped a README orphan line and a CLAUDE.md bullet.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… three misses
The walk skipped .git, dev and vendor but not .claude -- so it descended into
.claude/worktrees/, the directory this branch's own first commit gitignored
because `git worktree add` targets land there. Every nested checkout's copy of
the repo got scanned, and the guard reported internal/transport/transport.go as
a bypass and told the reader to widen carve-outs for files already carved out.
CI never saw it (fresh clone, no such directory); it failed only locally, on
the `go test ./...` CLAUDE.md mandates before pushing. Reproduced, fixed, and
re-checked with a fake worktree in place.
The misdirection from last round also survived, narrowed from one spelling to
two: converting a bypassing file to new(http.Client) or `var c http.Client`
still fired "the carve-outs are stale" about carve-outs that were true. Now
matching the http.Client TYPE in construction position -- composite literal,
new(), value var, value struct field -- which closes both, and closes the
embedded-client false negative at the same time. A *http.Client parameter is
deliberately not matched: mcpgateway.New takes one and does not construct it.
Two real detector bugs fixed rather than documented: the import loop had no
break, so a duplicate import ("net/http" plus nh "net/http") left httpName on
the last spelling and silently missed every use of the first -- verified by
removing the break again and watching the case pass. And a dot-import now
returns an error naming the problem instead of resolving to "." and quietly
matching nothing.
The limitation comment now states the real boundary rather than an understated
one: a hand-rolled RoundTripper driven directly (http.DefaultTransport.RoundTrip
included), a local type alias for http.Client, and third-party HTTP libraries
are what this does not catch. httpBypassFiles now says which entries are
bypasses and which one is the transport itself, since the loops treat them
identically and that is the trap for the next reader.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…miss Taking the code fix rather than documenting the limitation, since it is smaller than the note it would replace and removes an entry from the file's own "what this does NOT catch" list instead of adding one. The break made the FIRST net/http import win, which does not resolve a duplicate import -- it only chooses which of the two spellings goes invisible, and which one that is flips with import order. The mirror of the case I tested last round compiles, is gofmt-stable, contains a real bypass, and passed. httpName is now a set of every local name net/http is bound to, so no ordering hides a use, and the diagnostic reports the alias actually used at the call site. Both orderings and both spellings now fail; verified all four. The limitation list was understating its boundary, which is the failure this branch spent seven rounds correcting in four documents. It now names httputil.ReverseProxy (stdlib, so "third-party library" did not reach it), a defined local type as well as an alias, and says "any HTTP library other than net/http, stdlib or third-party" rather than "third-party". Re-proved after the change: mirror ordering fails, both orderings fail, the alias-as-sender case names the alias, .claude/worktrees stays skipped, a new bypass in cmd/ and an embedded http.Client under internal/ both fail, a dot-import errors, all three construction forms stay silent when a bypassing file is converted, and consolidating docs_openapi.go still fires the stale-carve-out message. The three real *http.Client parameter sites -- transport.go, mcpgateway.New, client.NewForTesting -- remain silent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
leet-c1
added a commit
that referenced
this pull request
Aug 27, 2026
#105's TestEveryFlagIsDocumented failed on --wait-min and --wait-stable, which this branch adds to `grants list`. It was right to: the doc lines were written during review and deliberately held back to avoid a four-way conflict on README.md and cmd/agents.md while five branches were in flight. The guard is what turned "I will remember to integrate these" into a build failure. README gets the recipe and the prose; agents.md gets the three facts an agent learns the hard way otherwise -- an empty result is stable so `--wait` plus zero rows is inconclusive rather than negative, the revoke direction has no flag because --wait-min is a floor not a ceiling, and --wait buffers so nothing reaches stdout until the set settles. Also reconciles agents.md's appOwners sentence with what `apps set-owners`' help now says: "observed empty on every app checked in testing" became "[] on all 46 apps of the second measurement pass, including the 45 that GET .../ownerids reported owners for". The old phrasing let a reader conclude those apps simply had no owners. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
leet-c1
added a commit
that referenced
this pull request
Aug 27, 2026
#105's TestEveryFlagIsDocumented failed on --wait-min and --wait-stable, which this branch adds to `grants list`. It was right to: the doc lines were written during review and deliberately held back to avoid a four-way conflict on README.md and cmd/agents.md while five branches were in flight. The guard is what turned "I will remember to integrate these" into a build failure. README gets the recipe and the prose; agents.md gets the three facts an agent learns the hard way otherwise -- an empty result is stable so `--wait` plus zero rows is inconclusive rather than negative, the revoke direction has no flag because --wait-min is a floor not a ceiling, and --wait buffers so nothing reaches stdout until the set settles. Also reconciles agents.md's appOwners sentence with what `apps set-owners`' help now says: "observed empty on every app checked in testing" became "[] on all 46 apps of the second measurement pass, including the 45 that GET .../ownerids reported owners for". The old phrasing let a reader conclude those apps simply had no owners. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merge this first. Its flag guard is the constraint the other open PRs must satisfy; none of them adds a flag, so they pass by construction.
1.
.claude/worktrees/is now gitignoredUntracked, it made every local build stamp
+dirty— into the version string, the User-Agent, and the MCP handshake — andgit add -Awould have recorded a nested checkout as a gitlink.Verified behaviourally in an isolated clone, not by interrogating
git check-ignore(which exits 0 for a negation match and has reported the opposite of the truth in this repo before):git status --porcelain?? .claude/worktrees/git add -A --dry-runc1i versionv0.5.3-…+dirtyv0.5.3-…2. Six flags shipped mentioned in neither
README.mdnorcmd/agents.mdapps set-owners --wait/--wait-timeout,auth token --json,mcp servers register --token-sharing/--source-app-id, andcompletion --no-descriptions.The cost isn't a typo — it's an agent rebuilding by hand what c1i already does. Two documented cases: one hand-rolled a pagination loop
--paginatehad always covered, shipping a duplicate published access profile to a live tenant; another burned seven attempts reverse-engineering MCP registration thatmcp servers register --tool-prefixalready did in the same version it was running.TestEveryFlagIsDocumentednow fails CI on any long flag absent from both docs;TestGlobalFlagsDocumentedInAgentsDocholds the persistent flags to both. Matching is boundary-anchored, so--waitis not satisfied by a doc mentioning only--wait-timeout. Both exemption maps are empty.3.
--debugand--max-retriesdo not reach thedocssubcommandscmd/docs_search.goandcmd/docs_openapi.gosend HTTP throughhttp.DefaultClient, so five subcommands —docs search,page,endpoints,endpoint,openapi— get no tracing, no retries, and neither the empty-path nor the redirect guard. Six documents said otherwise.Proving it took three attempts, which is why it's guarded rather than merely corrected: the first
--debugrun read a warm 24-hour cache and issued no request at all.TestDocumentedFlagScopeExceptionIsStillRealparses each file's AST, resolves every local namenet/httpis bound to, and flags package-level senders andhttp.Clientconstruction. It fails in both directions — a new bypass fails, and a listed file ceasing to bypass also fails, so when the underlying defect is fixed the docs can't keep warning about a hazard that no longer exists.Also corrected here, each verified against the code or the live spec:
mcp gatewaydoes honor both flags; theapps deleteendpoint is published; the token is not audience-scoped;docs search/pagehitapi.mintlify.com, not the C1 docs site.Known limits, stated rather than implied
The flag guard is per flag name, not per
(command, flag)pair — per-pair would demand a full matrix in both docs. The AST detector misseshttp.DefaultTransport.RoundTrip, a local alias or defined type forhttp.Client,httputil.ReverseProxy, and non-net/httplibraries. Both limits are recorded in the test files as settled decisions.Reviewed across seven rounds. Follow-ups filed: the three
http.DefaultClientsites themselves, andREADME.md:391, a fifth copy of the docs-host fact that this guard structurally cannot reach.Ledger: C158, C168.
🤖 Generated with Claude Code