From 311b709043a945479521aa53edf899063e77feb8 Mon Sep 17 00:00:00 2001 From: Dmitry Pleshakov Date: Thu, 8 Oct 2026 09:33:24 +0300 Subject: [PATCH 01/25] Record the command line rules for agent permissions (TASK-01) AI agent hosts such as Claude Code and opencode decide whether an agent may run a command by matching its text against allow, ask, and deny rules. A prefix rule is reliable only if the command path always opens the command line, and no rule can tell a writing command from a reading one by its action name: action names come from the REST API and form an open set, while Get Work Items Batch and Query By Wiql are POST requests that only read. The brief therefore requires flags to follow the command path, one form per command, and local command names distinct from API areas. Future write operations are gated by --allow-changes, and irreversible ones by --allow-irreversible in addition, so that one ask rule covers every change and one deny rule excludes the irreversible ones. The gate flags have no environment variable or config key, since a setting outside the command line is invisible to the rules. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01WCREyMqPZyHxuCRoy5FZqf --- ...6-10-08-tasks-command-shape-permissions.md | 130 ++++++++++++++++++ docs/project-brief.md | 17 ++- 2 files changed, 145 insertions(+), 2 deletions(-) create mode 100644 docs/2026-10-08-tasks-command-shape-permissions.md diff --git a/docs/2026-10-08-tasks-command-shape-permissions.md b/docs/2026-10-08-tasks-command-shape-permissions.md new file mode 100644 index 0000000..9a52dc4 --- /dev/null +++ b/docs/2026-10-08-tasks-command-shape-permissions.md @@ -0,0 +1,130 @@ +# 2026-10-08-tasks-command-shape-permissions.md + +**Status:** Active + +## Context + +Goal: the command line of tfscli is easy to govern with the permission rules +of AI agent hosts, such as Claude Code and opencode, now and after write +operations are added. Those rules match the text of a command: Claude Code +evaluates deny, then ask, then allow, and rule specificity does not change the +order; opencode applies the last matching rule. A rule written as a prefix +(`tfscli wit work-items get *`) is reliable only if the command path always +opens the command line, and no rule can tell a writing command from a reading +one by its action name, since action names come from the REST API and form an +open set (`add`, `create`, `update`, `delete`, `queue`, `run`, ...), while some +POST operations, such as `get-batch` and `query-by-wiql`, only read. + +Decisions already made: + +- **Flags follow the command path.** A token starting with `-` before the last + word of the command path is refused with a `config` error that shows the + corrected command. Today cobra accepts `tfscli --verbose wit work-items get 1`, + which a prefix rule does not match. Exceptions: `--help` and `--version` at + any level, and cobra's own `help` and `completion` commands. This is a + breaking change to the command line. +- **One form per command.** No command aliases, no abbreviated command or flag + names, no cobra prefix matching. +- **Local commands do not take API area names.** The names of local commands + (`auth`, `licenses`, `help`, `completion`) must not coincide with an area of + the REST API reference. +- **Whether a command changes server state is a property of the command**, not + of its flags: a reading command never gains a side effect through a flag. No + generic pass-through command (`invoke`, `api`) with an arbitrary method is + added; if one ever is, it falls under the gate flags below. +- **Gate flags for future write operations.** Every operation that changes + server state requires `--allow-changes`. An irreversible operation requires + both `--allow-changes` and `--allow-irreversible`, so that one ask rule on + `--allow-changes` covers every change and a deny rule on + `--allow-irreversible` excludes irreversible ones. The classification follows + the meaning of the operation, not its HTTP method. +- **Criterion of irreversibility:** an operation is irreversible when its + effect cannot be undone by another operation of the REST API. Deleting a work + item to the recycle bin is reversible (it can be restored); destroying it is + not. +- **The gate flags are flags only.** No environment variable or config key + supplies them, as an exception to the precedence chain, like the credential: + a setting outside the command line is invisible to permission rules. Short + forms are not provided. +- **The gate is checked before any request**, the `OPTIONS` negotiation + included. A missing gate flag is a `config` error naming the flag; no new + error category is added. +- **The gate covers server state only.** Local commands that change local + state live under `auth`, are interactive, and are excluded by a rule on + `tfscli auth *`. +- The gate flags deviate from "Follow TFS API structure"; the justification is + safety under agent permission rules, and it is recorded in the brief. +- Claude Code's documented matching was checked + (https://code.claude.com/docs/en/permissions); opencode's handling of + compound commands is not documented on https://opencode.ai/docs/permissions/ + and is not claimed in user documentation. + +--- + +### TASK-01 `brief` +**Description:** In `docs/project-brief.md`: add to Design Principles the rules +of the command line shape — flags after the command path with the exceptions +above, one form per command, local command names distinct from API areas, +server-state changes as a property of the command, no generic pass-through +command. Extend Non-Goals → Write operations with the gate flags: both flags, +the criterion of irreversibility, flags only, the check before any request, +the `config` error, and the justification of the deviation from the API +structure. Record in Configuration that the gate flags are outside the +precedence chain. +**Definition of done:** Every decision in the Context section that concerns the +product is stated in the brief; the brief does not contradict itself on +configuration precedence. +**Status:** Done + +### TASK-02 `architecture` +**Description:** In `docs/architecture.md`: describe where the check of flags +before the command path lives — an explicit check in `run()` built on +`root.Find`, since cobra accepts flags at any position and a +`PersistentPreRunE` on the root is silently replaced by a hook of the same +name on a child command; the tree-walking guard test; and, under "Notes for +future evolution", how writing commands will be marked so that the guard test +can require the gate flags of them. +**Definition of done:** The cli module description and the notes for future +evolution cover the three points. +**Status:** Pending + +### TASK-03 `claude-md` +**Description:** In `CLAUDE.md`: add short items to "Non-negotiable product +constraints" (gate flags, flags only, one form per command) and to "Command +shape" (flags after the command path), referring to the brief for the full +rules. +**Definition of done:** A reader of `CLAUDE.md` alone learns that flags follow +the command path, that aliases are excluded, and that write operations require +the gate flags. +**Status:** Pending + +### TASK-04 `flags-after-command` +**Description:** In `internal/cli`, refuse a token starting with `-` before the +last word of the command path with a `config` error that names the flag and +shows the corrected command, with the exceptions recorded in the Context +section. +**Definition of done:** Tests cover the refusal of `--verbose` and +`--api-version` placed before the path, at the root and between path words, and +the unchanged behaviour of `--version`, `--help` at every level, `help ...`, +and `completion ...`. `make check` passes. +**Status:** Pending + +### TASK-05 `command-tree-guard` +**Description:** Add a test that walks the whole command tree and fails when +any command declares `Aliases` or when `cobra.EnablePrefixMatching` is on. +**Definition of done:** The test fails when an alias is added to any command +and passes on the current tree. `make check` passes. +**Status:** Pending + +### TASK-06 `user-docs` +**Description:** In `README.md`, state under Usage that flags follow the +command, and add to "Use with an AI agent" permission rules for the current +read-only commands for Claude Code (Bash and PowerShell) and opencode: allow +`tfscli wit *` and `tfscli licenses`, deny `tfscli auth *`. In +`skills/tfscli/SKILL.md`, state that flags follow the command. Add an entry to +`CHANGELOG.md` per the `changelog` skill for the refusal of flags before the +command path, marked as a breaking change. +**Definition of done:** The README, the agent skill, and the changelog describe +the new rule; the README claims nothing about opencode beyond its documented +matching. `make check` passes. +**Status:** Pending diff --git a/docs/project-brief.md b/docs/project-brief.md index abd9888..1d1998f 100644 --- a/docs/project-brief.md +++ b/docs/project-brief.md @@ -40,7 +40,13 @@ An AI agent can call `tfscli`, read work item content, and use it meaningfully i ## Non-Goals -- **Write operations.** v1 is strictly read-only. Write operations are a future consideration, gated by PAT scopes. JSON output (`--json`) is a prerequisite for write support, since markdown conversion is lossy and not round-trippable to HTML. +- **Write operations.** v1 is strictly read-only. Write operations are a future consideration, gated by PAT scopes. JSON output (`--json`) is a prerequisite for write support, since markdown conversion is lossy and not round-trippable to HTML. When write operations are added, they are gated on the command line as follows, so that an AI agent host can require confirmation for every change with one permission rule: + - Every operation that changes server state requires the flag `--allow-changes`. An irreversible operation requires both `--allow-changes` and `--allow-irreversible`: a rule on `--allow-changes` then covers every change, and a rule on `--allow-irreversible` additionally excludes the irreversible ones. Without a required flag the command fails with a `config` error that names the flag. + - The classification follows the meaning of the operation, not its HTTP method: Get Work Items Batch and Query By Wiql are POST requests and read only, while queueing a build changes server state. An operation is irreversible when its effect cannot be undone by another operation of the REST API: deleting a work item to the recycle bin is reversible, since it can be restored, and destroying it is not. + - The gate flags are flags only, with no short form. No environment variable or config key supplies them, because a setting outside the command line is invisible to permission rules that match the command text; they are outside the precedence chain in "Configuration". + - The gate is checked before any request is sent, the `OPTIONS` request that negotiates the API version included. + - The gate covers server state only. Local commands that change local state, such as `auth login`, live under `auth` and are interactive. + - The gate flags are not parameters of the REST API and so deviate from "Follow TFS API structure". The deviation is justified by safety: whether a command changes server state cannot be told from its name, since action names come from the API and form an open set, while some POST operations only read. - **MCP server mode.** tfscli is a CLI utility, not an MCP server. If MCP integration is needed, it can be wrapped externally. - **Cloud Azure DevOps Services support.** Target is on-premises TFS / Azure DevOps Server. Cloud may work incidentally but is not tested or guaranteed. - **Custom query syntax.** No invented query language. WIQL queries are passed directly to the API. @@ -54,6 +60,11 @@ An AI agent can call `tfscli`, read work item content, and use it meaningfully i - The action is the name of the operation in the REST API reference with the resource name removed, in kebab-case: "Get Work Item" is `wit work-items get`, "List" is `wit work-items list`, "Get Work Items Batch" is `wit work-items get-batch`. The resource name is removed only where it names what the operation acts on; where it names something else, it stays: in "Query By Wiql" of the resource Wiql it names the input of the query, as "Id" does in "Query By Id", so the actions are `query-by-wiql` and `query-by-id`. - A path parameter that identifies the resource is a positional argument: `wit work-items get 12345` for `_apis/wit/workitems/{id}`. The project, the team, and the collection, which scope the request rather than identify the resource, are not: the collection comes from the credential, the project from `-p` or its defaults, and the team from `--team`. - A query or body parameter is a flag with the API name in kebab-case and without a leading `$`: `ids` is `--ids`, `errorPolicy` is `--error-policy`, `$expand` is `--expand`. A flag that is not given is not sent, so the server's default applies. +- **The command line can be governed by permission rules.** AI agent hosts, such as Claude Code and opencode, decide whether an agent may run a command by matching its text against allow, ask, and deny rules. tfscli keeps that text predictable: + - The command path opens the command line, and flags follow its last word: `tfscli wit work-items get --verbose 12345`, not `tfscli --verbose wit work-items get 12345`. A flag placed earlier is refused with a `config` error that shows the corrected command. The exceptions are `--help` and `--version`, which may follow any word of the path, and the built-in `help` and `completion` commands. + - Each command has exactly one form: no command aliases, no abbreviated command or flag names, no prefix matching of command names. + - The names of local commands (`auth`, `licenses`, `help`, `completion`) never coincide with an area of the REST API reference. + - Whether a command changes server state is a property of the command, never of its flags: a reading command does not gain a side effect through a flag. There is no generic pass-through command that sends an arbitrary request; should one be added, it falls under the gate flags described in "Non-Goals". - **Predictable error output.** Errors go to stderr with a machine-readable category and human-readable message. Exit code is non-zero. Format: `Error [category]: message (HTTP status)`. Error categories are a stable contract: existing categories are never removed or renamed; new categories may be added in future versions. - **Minimal configuration.** `auth login` stores the server URL, the collection, and the PAT. An optional config file stores an optional default project, the API version, and TLS settings. Environment variables and flags override config values. No other configuration needed. - **Clean output for AI consumption.** HTML in work item fields is converted to markdown. Metadata noise (URLs, internal IDs, revision details) is minimized in default output. @@ -88,13 +99,15 @@ All values overridable via environment variables (`TFSCLI_PROJECT`, `TFSCLI_API_ Priority: CLI flag > environment variable > config file > built-in default. +Two kinds of input are outside this chain: the credential (see "Authentication") and the gate flags of write operations (see "Non-Goals"), which are given on the command line only. + ## Command Format ``` tfscli [flags] [arguments] ``` -The area, the resource, and the action are named by the rule in "Follow TFS API structure". `tfscli auth login` is a local command of tfscli and lies outside this format. +The area, the resource, and the action are named by the rule in "Follow TFS API structure". Flags follow the action, as "The command line can be governed by permission rules" requires. `tfscli auth login` is a local command of tfscli and lies outside this format. Commands: From cba45ca9852229952515c97bfb95fe52db4318be Mon Sep 17 00:00:00 2001 From: Dmitry Pleshakov Date: Thu, 8 Oct 2026 09:34:19 +0300 Subject: [PATCH 02/25] Describe where the command line shape is enforced (TASK-02) Cobra accepts flags at any position, so keeping the command path at the start of the command line needs an explicit check in run, located with root.Find. A root PersistentPreRunE was rejected: cobra runs only the nearest such hook, and a child command defining its own would silently drop the check. Flags cobra handles itself (--help, --version) and flags the target command does not define are left to cobra. The notes for future evolution record how writing commands will be marked, so that the command tree test can require the gate flags of exactly those commands. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01WCREyMqPZyHxuCRoy5FZqf --- docs/2026-10-08-tasks-command-shape-permissions.md | 2 +- docs/architecture.md | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/2026-10-08-tasks-command-shape-permissions.md b/docs/2026-10-08-tasks-command-shape-permissions.md index 9a52dc4..c638ac7 100644 --- a/docs/2026-10-08-tasks-command-shape-permissions.md +++ b/docs/2026-10-08-tasks-command-shape-permissions.md @@ -86,7 +86,7 @@ future evolution", how writing commands will be marked so that the guard test can require the gate flags of them. **Definition of done:** The cli module description and the notes for future evolution cover the three points. -**Status:** Pending +**Status:** Done ### TASK-03 `claude-md` **Description:** In `CLAUDE.md`: add short items to "Non-negotiable product diff --git a/docs/architecture.md b/docs/architecture.md index 0db7ab5..7a65f2f 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -195,7 +195,7 @@ This section describes the conceptual module structure, their responsibilities, ### Modules and responsibilities -- **cli** — cobra commands, nested as area, resource, and action after the REST API reference (`wit` → `work-items` → `get`); persistent flags (`--verbose`, config overrides); orchestrates the chain config → apiclient → domain → printer → exit. Owns the markdown printer functions (one per resource and shape, such as a single work item and several) plus small per-kind scalar formatters for identity and datetime values used by those printers. No `Renderer` interface in v1 — printers are plain functions. When `--json` is added later, the interface and a second renderer can be introduced here without touching the domain layer. +- **cli** — cobra commands, nested as area, resource, and action after the REST API reference (`wit` → `work-items` → `get`); persistent flags (`--verbose`, config overrides); orchestrates the chain config → apiclient → domain → printer → exit. Owns the markdown printer functions (one per resource and shape, such as a single work item and several) plus small per-kind scalar formatters for identity and datetime values used by those printers. No `Renderer` interface in v1 — printers are plain functions. When `--json` is added later, the interface and a second renderer can be introduced here without touching the domain layer. Enforces the command line shape that "The command line can be governed by permission rules" in `project-brief.md` requires. Cobra accepts flags at any position, so `run` checks the arguments before executing the tree: it locates the target command with `root.Find`, and a flag token placed before the last word of the command path fails with a `config` error that shows the corrected command. The check is explicit rather than a root `PersistentPreRunE`, because cobra runs only the nearest such hook and a child command defining its own would silently drop it. `--help`, `-h`, and `--version` before the end of the path, a flag the target command does not define, and the `--` terminator skip the check, leaving them to cobra's own handling. A test walks the whole command tree and fails on any command alias or on cobra's prefix matching, so that each command keeps exactly one form. - **config** — resolves the credential (`URL`, `Collection`, and `PAT`; a PAT is issued for one collection) from `TFSCLI_AUTH` or `$XDG_DATA_HOME/tfscli/auth.json`, never from a mix of sources; loads the optional `$XDG_CONFIG_HOME/tfscli/config.json`, applies environment overrides (`TFSCLI_*`), applies cobra flag overrides; validates that all required fields are set for the command being run. Writes `auth.json` for `auth login` (mode `0600`, directory `0700`). Holds `URL`, `Collection`, `PAT`, `Project`, `APIVersion`, `InsecureSkipVerify`, `CABundle`. Never logs PAT. - **apiclient** — wraps `net/http`. Builds URLs from `Config.URL + Collection + path` and adds `api-version`. A configured `APIVersion` is sent as it is. Otherwise the version is negotiated per request, as the official SDKs do: `OPTIONS {URL}/{Collection}/_apis` lists every resource location with its versions, the caller names its location by id, and the request carries the location's released version, or `{maxVersion}-preview.{resourceVersion}` when it has none — the version the server chooses for a request without one. The response is not cached. A 401 or a transport failure on `OPTIONS` fails the request; any other failure sends it without a version and notes why through `Logger.Info`, and a refusal of that request for the missing version gets the settings to use in its message. A call without a location, the login check, neither negotiates nor sends a version. Sets `Authorization: Basic base64(":")` on every request. Configures TLS using `CABundle` (appended to system root pool) and `InsecureSkipVerify`. Hooks the logger via a `RoundTripper` — that transport is the single call site of `LogRequest`, so every outgoing request is logged exactly once and no other module logs HTTP traffic. Classifies HTTP outcomes into the stable error categories defined in the brief (`auth`, `not_found`, `forbidden`, `server`, `config`, `network`). - **workitem** — domain logic for Work Items: Get Work Item, Work Items - List, and Get Work Items Batch. Owns the `WorkItem`/`Field` types, the `BatchRequest`/`Batch` types for reading several work items, and the allowlist of fields known to contain HTML (`System.Description`, `Microsoft.VSTS.TCM.ReproSteps`, `Microsoft.VSTS.TCM.SystemInfo`, `Microsoft.VSTS.Common.AcceptanceCriteria`, …). After unmarshalling, tags each field with its `FieldKind`. For several work items, keeps the server's order and reports the requested ids the server did not return. Returns raw values — does not perform markdown conversion. @@ -348,6 +348,7 @@ type Result struct { - **New API domain** (repos, builds): add a new domain module and register its resource command under the area command, adding the area command (e.g. `git`) if it does not exist yet. No edits to `apiclient`, `config`, or `tfserr`. - **`--json` output**: introduce a `Renderer` interface in the cli layer; add a `JSONRenderer` that consumes `*WorkItem` directly. The markdown printer becomes the other implementation. Local refactor confined to the cli layer. - **TFS-specific HTML quirks** (mentions, attachment links, Word paste leftovers): add rules in `htmlmd` plus golden tests on real samples. Recorded as the `html-quirks` entry in `docs/backlog.md`. +- **Write operations**: a command that changes server state declares it in its cobra `Annotations` — reversible or irreversible, per the criterion in `project-brief.md` — and registers the gate flags through one helper in the cli layer that defines `--allow-changes`, and `--allow-irreversible` for an irreversible command, and refuses a missing one with a `config` error before any request, the `OPTIONS` negotiation included. The command tree test is extended to require that every annotated command defines its gate flags and that no unannotated command defines them. --- From 47fc5c36873d26987be2d9a7627a17c3b16dc61a Mon Sep 17 00:00:00 2001 From: Dmitry Pleshakov Date: Thu, 8 Oct 2026 09:34:35 +0300 Subject: [PATCH 03/25] Add the command line rules to CLAUDE.md (TASK-03) The rules are listed among the non-negotiable constraints so that a future change is checked against them before it is made, with the full wording left to the brief. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01WCREyMqPZyHxuCRoy5FZqf --- CLAUDE.md | 4 +++- docs/2026-10-08-tasks-command-shape-permissions.md | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 3157e46..168f4dd 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -22,6 +22,8 @@ These come from `project-brief.md` and override casual feature requests. If a ch - **PAT auth only.** No SSPI or NTLM. A PAT is issued for one collection, so the server URL, the collection, and the PAT are stored together and always come from one source: `$XDG_DATA_HOME/tfscli/auth.json` (default `~/.local/share/tfscli/auth.json`, mode `0600`, written only by the interactive `tfscli auth login`) or `TFSCLI_AUTH` holding the same JSON, which takes precedence. No flag, separate env var, or config key may supply the URL, the collection, or the token. No OS keychain unless an organisation requires one. - **Read-only in v1.** Write operations are explicitly out of scope. They additionally depend on JSON output landing first, because markdown is lossy. +- **Write operations are gated on the command line.** When they are added, every operation that changes server state requires `--allow-changes`, and an irreversible one (its effect cannot be undone by another API operation) requires `--allow-irreversible` as well. Writing is a property of the command, never of a flag of a reading command. The gate flags have no short form, environment variable, or config key, and are checked before any request. See "Write operations" in Non-Goals of `project-brief.md`. +- **The command line stays matchable by agent permission rules.** Flags follow the last word of the command path; each command has one form (no aliases, no abbreviations, no prefix matching); local command names never coincide with REST API areas; no generic pass-through command. See "The command line can be governed by permission rules" in `project-brief.md`. - **Markdown is the default output.** HTML fields (Description, ReproSteps, etc.) are converted via `github.com/JohannesKaufmann/html-to-markdown/v2`. JSON output is a planned `--json` flag, not present in v1. - **Mirror TFS REST API structure.** Command hierarchy, parameter names, and behaviour follow the API. Any deviation needs explicit usability justification — don't invent new abstractions (no custom query syntax, no synthetic batch endpoints, no caching). - **Stateless.** No daemon, no background process, no on-disk cache. Every call hits the server. @@ -38,7 +40,7 @@ These come from `project-brief.md` and override casual feature requests. If a ch tfscli [flags] [arguments] ``` -The area and the resource are the segments of the operation's page path in the REST API reference (`.../wit/work-items/get-work-item` is `wit work-items get`); the action is the operation name without the resource name. `tfscli auth login` and `tfscli licenses` are local commands and lie outside this scheme. The full rule is in "Follow TFS API structure" in `project-brief.md`. +The area and the resource are the segments of the operation's page path in the REST API reference (`.../wit/work-items/get-work-item` is `wit work-items get`); the action is the operation name without the resource name. Flags follow the action: a flag placed before the end of the command path, such as `tfscli --verbose wit work-items get 1`, is refused. `tfscli auth login` and `tfscli licenses` are local commands and lie outside this scheme. The full rule is in "Follow TFS API structure" in `project-brief.md`. Commands (from the brief): diff --git a/docs/2026-10-08-tasks-command-shape-permissions.md b/docs/2026-10-08-tasks-command-shape-permissions.md index c638ac7..d72eb1e 100644 --- a/docs/2026-10-08-tasks-command-shape-permissions.md +++ b/docs/2026-10-08-tasks-command-shape-permissions.md @@ -96,7 +96,7 @@ rules. **Definition of done:** A reader of `CLAUDE.md` alone learns that flags follow the command path, that aliases are excluded, and that write operations require the gate flags. -**Status:** Pending +**Status:** Done ### TASK-04 `flags-after-command` **Description:** In `internal/cli`, refuse a token starting with `-` before the From 16ed53460bc354614e91ffd3cd463b2e260c88aa Mon Sep 17 00:00:00 2001 From: Dmitry Pleshakov Date: Thu, 8 Oct 2026 09:38:18 +0300 Subject: [PATCH 04/25] Refuse flags placed before the end of the command path (TASK-04) An agent permission rule such as "tfscli wit work-items get *" matches the command text by its prefix, so "tfscli --verbose wit work-items get 1" slipped past it. run now locates the target command with root.Find and refuses a flag token before the last word of its path with a config error that shows the corrected command, before any request is sent. The check yields to cobra wherever it cannot place an argument: an unknown command, a flag the target command does not define (--help and --version included, since cobra adds them only on execution), the "--" terminator, and a stray positional argument. Moving an unknown flag would not make the command valid, so cobra's own error is the better one. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01WCREyMqPZyHxuCRoy5FZqf --- ...6-10-08-tasks-command-shape-permissions.md | 2 +- internal/cli/cli.go | 4 + internal/cli/flagorder.go | 115 ++++++++++++++++++ internal/cli/flagorder_test.go | 110 +++++++++++++++++ 4 files changed, 230 insertions(+), 1 deletion(-) create mode 100644 internal/cli/flagorder.go create mode 100644 internal/cli/flagorder_test.go diff --git a/docs/2026-10-08-tasks-command-shape-permissions.md b/docs/2026-10-08-tasks-command-shape-permissions.md index d72eb1e..58e2c48 100644 --- a/docs/2026-10-08-tasks-command-shape-permissions.md +++ b/docs/2026-10-08-tasks-command-shape-permissions.md @@ -107,7 +107,7 @@ section. `--api-version` placed before the path, at the root and between path words, and the unchanged behaviour of `--version`, `--help` at every level, `help ...`, and `completion ...`. `make check` passes. -**Status:** Pending +**Status:** Done ### TASK-05 `command-tree-guard` **Description:** Add a test that walks the whole command tree and fails when diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 521f428..85aed7a 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -43,6 +43,10 @@ func run(b build, args []string, stdin prompter, stdout, stderr io.Writer) int { root := newRoot(b, stdin, stdout, stderr) root.SetArgs(args) + if err := checkFlagOrder(root, args); err != nil { + tfserr.Print(err, stderr) + return tfserr.ExitCode(err) + } if err := root.ExecuteContext(ctx); err != nil { tfserr.Print(categorized(err), stderr) return tfserr.ExitCode(err) diff --git a/internal/cli/flagorder.go b/internal/cli/flagorder.go new file mode 100644 index 0000000..ead8235 --- /dev/null +++ b/internal/cli/flagorder.go @@ -0,0 +1,115 @@ +package cli + +import ( + "fmt" + "strconv" + "strings" + + "github.com/spf13/cobra" + + "github.com/dpleshakov/tfscli/internal/tfserr" +) + +// checkFlagOrder refuses a flag placed before the last word of the command +// path, such as --verbose in "tfscli --verbose wit work-items get 1". Agent +// permission rules match the command text by its prefix, so the command path +// has to open the command line; cobra itself accepts flags at any position. +// +// Whatever the check cannot place is left to cobra: an unknown command, a +// flag the target command does not define (--help and --version among them, +// since cobra adds those only when it executes), the "--" terminator, and an +// argument that is neither a flag nor a word of the path. +func checkFlagOrder(root *cobra.Command, args []string) error { + cmd, _, err := root.Find(args) + if err != nil || cmd == root { + return nil + } + path := strings.Fields(cmd.CommandPath())[1:] + + var misplaced, moved []string + i, next := 0, 0 + for ; i < len(args) && next < len(path); i++ { + arg := args[i] + if arg == path[next] { + next++ + continue + } + if arg == "--" || !strings.HasPrefix(arg, "-") { + return nil + } + name, short, inline := splitFlag(arg) + found, takesValue := lookupFlag(cmd, name, short) + if !found { + return nil + } + misplaced = append(misplaced, strings.SplitN(arg, "=", 2)[0]) + moved = append(moved, arg) + if takesValue && !inline && i+1 < len(args) { + i++ + moved = append(moved, args[i]) + } + } + if len(misplaced) == 0 { + return nil + } + + verb := "comes" + if len(misplaced) > 1 { + verb = "come" + } + corrected := append(append(append([]string{root.Name()}, path...), moved...), args[i:]...) + return &tfserr.Error{ + Category: tfserr.Config, + Message: fmt.Sprintf("flags must follow the command %q, and %s %s before its end (run it as: %s)", + cmd.CommandPath(), strings.Join(misplaced, ", "), verb, shellJoin(corrected)), + } +} + +// splitFlag takes a flag token apart: "--name", "--name=value", "-n", or +// "-nvalue". inline reports whether the token carries its value. +func splitFlag(arg string) (name string, short, inline bool) { + if body, ok := strings.CutPrefix(arg, "--"); ok { + name, _, inline = strings.Cut(body, "=") + return name, false, inline + } + body := strings.TrimPrefix(arg, "-") + if len(body) > 1 { + return body[:1], true, true + } + return body, true, false +} + +// lookupFlag finds a flag of cmd by name or shorthand, its own or inherited +// from a parent, and reports whether it takes a value: a flag without a value +// for its bare form, such as --project, consumes the next argument, while a +// boolean such as --verbose does not. +func lookupFlag(cmd *cobra.Command, name string, short bool) (found, takesValue bool) { + local, inherited := cmd.Flags(), cmd.InheritedFlags() + f := local.Lookup(name) + if f == nil { + f = inherited.Lookup(name) + } + if short { + f = local.ShorthandLookup(name) + if f == nil { + f = inherited.ShorthandLookup(name) + } + } + if f == nil { + return false, false + } + return true, f.NoOptDefVal == "" +} + +// shellJoin joins the arguments into a command line, quoting those a shell +// would split or drop. +func shellJoin(args []string) string { + quoted := make([]string, len(args)) + for i, a := range args { + if a == "" || strings.ContainsAny(a, " \t\"'") { + a = strconv.Quote(a) + } + quoted[i] = a + } + return strings.Join(quoted, " ") +} diff --git a/internal/cli/flagorder_test.go b/internal/cli/flagorder_test.go new file mode 100644 index 0000000..2d36fc5 --- /dev/null +++ b/internal/cli/flagorder_test.go @@ -0,0 +1,110 @@ +package cli + +import ( + "net/http" + "strings" + "testing" +) + +// A flag before the end of the command path would slip past an agent +// permission rule written as a prefix, so it is refused before any request. +func TestFlagBeforeTheCommandPathIsRefused(t *testing.T) { + tests := []struct { + name string + args []string + want string + }{ + { + name: "persistent flag at the root", + args: []string{"--verbose", "wit", "work-items", "get", "-p", "MyProject", "12345"}, + want: `Error [config]: flags must follow the command "tfscli wit work-items get", and --verbose comes before its end (run it as: tfscli wit work-items get --verbose -p MyProject 12345)` + "\n", + }, + { + name: "flag with a separate value between path words", + args: []string{"wit", "--api-version", "7.1", "work-items", "get", "-p", "MyProject", "12345"}, + want: `Error [config]: flags must follow the command "tfscli wit work-items get", and --api-version comes before its end (run it as: tfscli wit work-items get --api-version 7.1 -p MyProject 12345)` + "\n", + }, + { + name: "flag with an inline value and a quoted argument", + args: []string{"--api-version=7.1", "wit", "wiql", "query-by-wiql", "--query", "SELECT [System.Id] FROM WorkItems"}, + want: `Error [config]: flags must follow the command "tfscli wit wiql query-by-wiql", and --api-version comes before its end (run it as: tfscli wit wiql query-by-wiql --api-version=7.1 --query "SELECT [System.Id] FROM WorkItems")` + "\n", + }, + { + name: "flags of the action before its name", + args: []string{"wit", "work-items", "-p", "MyProject", "--verbose", "get", "12345"}, + want: `Error [config]: flags must follow the command "tfscli wit work-items get", and -p, --verbose come before its end (run it as: tfscli wit work-items get -p MyProject --verbose 12345)` + "\n", + }, + { + name: "shorthand with an attached value", + args: []string{"wit", "work-items", "-pMyProject", "get", "12345"}, + want: `Error [config]: flags must follow the command "tfscli wit work-items get", and -pMyProject comes before its end (run it as: tfscli wit work-items get -pMyProject 12345)` + "\n", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s := newServer(t, http.StatusOK, workItemResponse) + + stdout, stderr, code := execute(t, s, tt.args...) + + if code != 1 { + t.Errorf("exit code = %d, want 1", code) + } + if stderr != tt.want { + t.Errorf("stderr = %q, want %q", stderr, tt.want) + } + if stdout != "" { + t.Errorf("stdout = %q, want nothing", stdout) + } + if s.calls != 0 || s.options != 0 { + t.Errorf("the server was called %d times and asked for options %d times, want no request at all", s.calls, s.options) + } + }) + } +} + +// The flags cobra handles itself and its built-in commands keep working +// wherever they are placed, since they send no request. +func TestHelpAndVersionAreNotRefusedBeforeThePath(t *testing.T) { + tests := []struct { + args []string + want string + }{ + {args: []string{"--version"}, want: "tfscli dev (unknown)"}, + {args: []string{"--help"}, want: "Available Commands:"}, + {args: []string{"-h", "wit"}, want: "Available Commands:"}, + {args: []string{"wit", "--help"}, want: "Available Commands:"}, + {args: []string{"wit", "work-items", "--help"}, want: "Available Commands:"}, + {args: []string{"wit", "work-items", "get", "--help"}, want: "--fields"}, + {args: []string{"wit", "--help", "work-items", "get"}, want: "Available Commands:"}, + {args: []string{"help", "wit", "work-items", "get"}, want: "--fields"}, + {args: []string{"--verbose", "help", "wit"}, want: "Available Commands:"}, + {args: []string{"completion", "powershell"}, want: "Register-ArgumentCompleter"}, + } + + for _, tt := range tests { + t.Run(strings.Join(tt.args, " "), func(t *testing.T) { + stdout, stderr, code := execute(t, nil, tt.args...) + + if code != 0 { + t.Fatalf("exit code = %d, want 0 (stderr: %s)", code, stderr) + } + if !strings.Contains(stdout, tt.want) { + t.Errorf("stdout does not contain %q:\n%s", tt.want, stdout) + } + }) + } +} + +// A flag the target command does not define is reported by cobra as unknown, +// not as misplaced: moving it would not make the command valid. +func TestUnknownFlagBeforeThePathIsLeftToCobra(t *testing.T) { + _, stderr, code := execute(t, nil, "--nope=1", "wit", "work-items", "get", "-p", "MyProject", "12345") + + if code != 1 { + t.Errorf("exit code = %d, want 1", code) + } + if want := "Error [config]: unknown flag: --nope"; !strings.HasPrefix(stderr, want) { + t.Errorf("stderr = %q, want it to start with %q", stderr, want) + } +} From 2a93c5a1da74b890a316f5a0bb96197f662e5bf0 Mon Sep 17 00:00:00 2001 From: Dmitry Pleshakov Date: Thu, 8 Oct 2026 09:39:05 +0300 Subject: [PATCH 05/25] Guard against command aliases and prefix matching (TASK-05) Agent permission rules match the command text, so a second name for a command would let it run under a form no rule was written for. The rule is recorded in the brief; this test makes make check enforce it for every command in the tree. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01WCREyMqPZyHxuCRoy5FZqf --- ...6-10-08-tasks-command-shape-permissions.md | 2 +- internal/cli/tree_test.go | 28 +++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 internal/cli/tree_test.go diff --git a/docs/2026-10-08-tasks-command-shape-permissions.md b/docs/2026-10-08-tasks-command-shape-permissions.md index 58e2c48..188ea55 100644 --- a/docs/2026-10-08-tasks-command-shape-permissions.md +++ b/docs/2026-10-08-tasks-command-shape-permissions.md @@ -114,7 +114,7 @@ and `completion ...`. `make check` passes. any command declares `Aliases` or when `cobra.EnablePrefixMatching` is on. **Definition of done:** The test fails when an alias is added to any command and passes on the current tree. `make check` passes. -**Status:** Pending +**Status:** Done ### TASK-06 `user-docs` **Description:** In `README.md`, state under Usage that flags follow the diff --git a/internal/cli/tree_test.go b/internal/cli/tree_test.go new file mode 100644 index 0000000..8dc3afa --- /dev/null +++ b/internal/cli/tree_test.go @@ -0,0 +1,28 @@ +package cli + +import ( + "io" + "testing" + + "github.com/spf13/cobra" +) + +// Agent permission rules match the command text, so each command must have +// exactly one form: an alias or prefix matching would let a command run under +// a name no rule was written for. +func TestEveryCommandHasOneForm(t *testing.T) { + if cobra.EnablePrefixMatching { + t.Error("cobra.EnablePrefixMatching is on; command names must be matched in full") + } + + var walk func(cmd *cobra.Command) + walk = func(cmd *cobra.Command) { + if len(cmd.Aliases) > 0 { + t.Errorf("%q has aliases %q; a command has exactly one name", cmd.CommandPath(), cmd.Aliases) + } + for _, child := range cmd.Commands() { + walk(child) + } + } + walk(newRoot(testBuild, noTerminal{}, io.Discard, io.Discard)) +} From 144b0bf8d8c3ba625ca7d819fe56c97fdbe9abb2 Mon Sep 17 00:00:00 2001 From: Dmitry Pleshakov Date: Thu, 8 Oct 2026 09:40:08 +0300 Subject: [PATCH 06/25] Document the command line rule and agent permission rules (TASK-06) The README states that flags follow the command and gives ready rules for Claude Code and opencode that allow the reading commands and deny auth, which an agent has no reason to run. The opencode rules say only what its documentation does: the last matching rule wins. The agent skill tells the agent to keep flags after the command rather than rely on the error, and the changelog records the refusal as a change to the command line. Closes docs/archive/2026-10-08-tasks-command-shape-permissions.md Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01WCREyMqPZyHxuCRoy5FZqf --- CHANGELOG.md | 3 ++ README.md | 50 +++++++++++++++++++ ...6-10-08-tasks-command-shape-permissions.md | 4 +- skills/tfscli/SKILL.md | 7 +++ 4 files changed, 62 insertions(+), 2 deletions(-) rename docs/{ => archive}/2026-10-08-tasks-command-shape-permissions.md (99%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 70e6c81..7c6c8eb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed +- A flag placed before the end of the command path, such as `--verbose` in `tfscli --verbose wit work-items get 12345`, is now refused with a `config` error that shows the command with the flag moved after it, where it used to be accepted; flags now always follow the command, so that agent permission rules can match the command by its name. + ## [0.1.1] - 2026-10-06 ### Added diff --git a/README.md b/README.md index 9937aee..552d18f 100644 --- a/README.md +++ b/README.md @@ -152,6 +152,54 @@ those settings first. An agent without skill support can be pointed at the same file directly: it is plain markdown under a short YAML header. +### Permission rules + +Both agents decide whether a shell command may run by matching its text against +rules. Every tfscli command starts with its full name, `tfscli +`, followed by flags and arguments, and each command has exactly one +name, so a rule written as a prefix covers the command whatever its flags. The +current commands only read; `auth login` is interactive and is not for an +agent to run. + +Claude Code, in `.claude/settings.json` or `~/.claude/settings.json`; the +`PowerShell` rules apply where the agent runs commands through PowerShell: + +```json +{ + "permissions": { + "allow": [ + "Bash(tfscli wit *)", + "Bash(tfscli licenses)", + "PowerShell(tfscli wit *)", + "PowerShell(tfscli licenses)" + ], + "deny": [ + "Bash(tfscli auth *)", + "PowerShell(tfscli auth *)" + ] + } +} +``` + +opencode, in `opencode.json`. The last matching rule wins, so place these after +any catch-all `"*"` rule: + +```json +{ + "permission": { + "bash": { + "tfscli wit *": "allow", + "tfscli licenses": "allow", + "tfscli auth *": "deny" + } + } +} +``` + +A rule matches the program name as the agent types it: a call through +`tfscli.exe` or a full path does not match `tfscli wit *` and is asked about +instead. + ## Configuration Every setting apart from the credential: @@ -213,6 +261,8 @@ tfscli [flags] [arguments] Commands are named after the REST API reference: the area and the resource are the segments of the operation's page path, so Get Work Item, documented under `.../wit/work-items/get-work-item`, is `wit work-items get`. `auth login` and `licenses` are local to tfscli and have no counterpart in the API. +Flags follow the command: `tfscli wit work-items get --verbose -p MyProject 12345`, never `tfscli --verbose wit work-items get ...`. A flag placed before the last word of the command is refused with a `config` error that shows the command with the flag moved, and nothing is sent to the server. `--help` and `--version` are the exceptions. The rule keeps the command at the start of the command line, where agent permission rules look for it (see "Permission rules"). + `tfscli --version` prints the version and the commit it was built from; `tfscli --help`, and `--help` on any command, lists the flags. `tfscli licenses` prints the components built into the binary — tfscli itself, the Go standard library, and the third-party modules — followed by the license text of each. ### `wit work-items get` diff --git a/docs/2026-10-08-tasks-command-shape-permissions.md b/docs/archive/2026-10-08-tasks-command-shape-permissions.md similarity index 99% rename from docs/2026-10-08-tasks-command-shape-permissions.md rename to docs/archive/2026-10-08-tasks-command-shape-permissions.md index 188ea55..abd9ef2 100644 --- a/docs/2026-10-08-tasks-command-shape-permissions.md +++ b/docs/archive/2026-10-08-tasks-command-shape-permissions.md @@ -1,6 +1,6 @@ # 2026-10-08-tasks-command-shape-permissions.md -**Status:** Active +**Status:** Archived ## Context @@ -127,4 +127,4 @@ command path, marked as a breaking change. **Definition of done:** The README, the agent skill, and the changelog describe the new rule; the README claims nothing about opencode beyond its documented matching. `make check` passes. -**Status:** Pending +**Status:** Done diff --git a/skills/tfscli/SKILL.md b/skills/tfscli/SKILL.md index b2ac6db..1591a1f 100644 --- a/skills/tfscli/SKILL.md +++ b/skills/tfscli/SKILL.md @@ -131,6 +131,13 @@ so `-p` can carry everything a call needs beyond the credential. ## The commands +Write the full command name first and every flag after it: +`tfscli wit work-items get --verbose -p `. A flag before the last +word of the command, as in `tfscli --verbose wit work-items get ...`, is refused +with a `config` error that shows the corrected command; the user's permission +rules match the command by its name, so keep that order rather than relying on +the error. + ### One work item ``` From 022c6ee7e11ef090a25dd57ba467520f2d783d89 Mon Sep 17 00:00:00 2001 From: Dmitry Pleshakov Date: Thu, 8 Oct 2026 10:03:37 +0300 Subject: [PATCH 07/25] Find the command path by walking the arguments (TASK-01) cobra's Find decides whether a flag takes a value only from the flags of the command it stands on. A flag unknown there swallows the next word: --help before cobra registers it, or a flag of an action placed at the level of its group. "tfscli --help wit work-items get" failed with an unknown command, and "tfscli wit wiql --time-precision query-by-wiql" printed the help of wiql with exit 0 without sending the query, a false success the flag order check let through because Find stopped at the group. The check now walks the arguments from the root and decides whether a flag takes a value by a flag of that name on the current command or anywhere below it, so it always reaches the real command. Every flag before the end of the path is refused, --help included; --version, which only the root defines, gets a message of its own. help and completion are registered when the tree is built, so they are walked like any other command rather than exempted. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01WCREyMqPZyHxuCRoy5FZqf --- docs/2026-10-08-tasks-command-path-walk.md | 77 +++++++++++++ internal/cli/cli.go | 4 + internal/cli/flagorder.go | 124 ++++++++++++++------- internal/cli/flagorder_test.go | 69 ++++++++---- 4 files changed, 213 insertions(+), 61 deletions(-) create mode 100644 docs/2026-10-08-tasks-command-path-walk.md diff --git a/docs/2026-10-08-tasks-command-path-walk.md b/docs/2026-10-08-tasks-command-path-walk.md new file mode 100644 index 0000000..ff67184 --- /dev/null +++ b/docs/2026-10-08-tasks-command-path-walk.md @@ -0,0 +1,77 @@ +# 2026-10-08-tasks-command-path-walk.md + +**Status:** Active + +## Context + +The check that refuses flags before the end of the command path +(`docs/archive/2026-10-08-tasks-command-shape-permissions.md`) locates the +target command with cobra's `Find`. `Find` removes flags while it looks for +subcommands, and decides whether a flag takes a value only from the flags of +the command it stands on at that moment (`stripFlags` in `command.go` of cobra +1.10.2). A flag unknown there is assumed to take a value and swallows the next +word. Two kinds of flags are unknown at that point: `--help`, `-h`, and +`--version`, which cobra registers only when it executes the command found, +and the flags of an action placed at the level of its group. Observed on the +built binary: + +| Invocation | Word swallowed | Result | +|---|---|---| +| `tfscli --help wit work-items get` | `wit` | `unknown command "work-items"`, exit 1 | +| `tfscli wit --help work-items get` | `work-items` | help of `wit`, not of `get`, exit 0 | +| `tfscli wit wiql --time-precision query-by-wiql --query X` | `query-by-wiql` | help of `wiql`, exit 0, no request | + +In the third case `Find` stops at the group, so the check sees the flag after +the path and lets it pass; the group ignores unknown flags +(`FParseErrWhitelist` in `newGroupCmd`) and prints its help with exit 0, a +false success. gh 2.102.0 avoids the first two for `--help` alone by declaring +it as a persistent flag on its root, and still fails on `-h`; it reports a flag +of an action placed before the action as an unknown flag, since its groups do +not ignore unknown flags. + +Decisions already made: + +- The check walks the arguments from the root itself instead of using `Find`. + A word naming a subcommand of the current command is a step of the path. A + word that follows a flag taking a value is that value, even when it names a + subcommand, as pflag would parse it. Whether a flag takes a value is decided + by a flag of that name or shorthand on the current command, inherited by it, + or defined anywhere below it; `--help`, `-h`, and `--version` take none; a + flag found nowhere is taken to have no value. +- cobra's `help` and `completion` commands are registered when the tree is + built, so that the walk treats them as ordinary commands and they need no + exception. +- Every flag before the last word of the path is refused, `--help` and `-h` + included, with the corrected command. `--version` belongs to the root only, + so moving it would make an unknown flag; it is refused with a message that + names `tfscli --version`. +- An invocation without a command path, such as `tfscli --help` or + `tfscli --version`, is not affected. +- The whitelist of unknown flags in `newGroupCmd` stays: after the check no + flag reaches a group before a valid action, and the whitelist keeps an + unknown action name reported as an unknown command. + +--- + +### TASK-01 `path-walk` +**Description:** Replace the use of `Find` in `checkFlagOrder` with the walk +described in the Context section; register the `help` and `completion` +commands in `newRoot`; refuse `--version` before the end of the path with its +own message. +**Definition of done:** Tests cover every row of the table above, `-h` before +the path, a flag of an action placed at the level of its group, `help` and +`completion` with a flag before them, `--version` before a path, and a value +that names a subcommand. `make check` passes. +**Status:** Done + +### TASK-02 `docs` +**Description:** Update the wording on exceptions in `docs/project-brief.md`, +`docs/architecture.md`, `CLAUDE.md`, `README.md`, and +`skills/tfscli/SKILL.md`: `--help` follows the command path like any other +flag, and only an invocation without a path, such as `tfscli --help` or +`tfscli --version`, has none. Update the unreleased `CHANGELOG.md` entry to +match. +**Definition of done:** No document states that `--help` may precede the end +of the path, or that `help` and `completion` are exceptions; the architecture +describes the walk instead of `Find`. `make check` passes. +**Status:** Pending diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 85aed7a..f08cebd 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -101,6 +101,10 @@ func newRoot(b build, stdin prompter, stdout, stderr io.Writer) *cobra.Command { root.AddCommand(newAuthCmd(g)) root.AddCommand(newWitCmd(g)) root.AddCommand(newLicensesCmd(g)) + // Cobra adds help and completion only when it executes the tree; adding + // them here lets checkFlagOrder walk them like any other command. + root.InitDefaultHelpCmd() + root.InitDefaultCompletionCmd() return root } diff --git a/internal/cli/flagorder.go b/internal/cli/flagorder.go index ead8235..0762bde 100644 --- a/internal/cli/flagorder.go +++ b/internal/cli/flagorder.go @@ -15,49 +15,67 @@ import ( // permission rules match the command text by its prefix, so the command path // has to open the command line; cobra itself accepts flags at any position. // -// Whatever the check cannot place is left to cobra: an unknown command, a -// flag the target command does not define (--help and --version among them, -// since cobra adds those only when it executes), the "--" terminator, and an -// argument that is neither a flag nor a word of the path. +// The path is found by walking the arguments from the root rather than with +// cobra's Find. Find decides whether a flag takes a value only from the flags +// of the command it stands on, so a flag unknown there — --help before cobra +// registers it, or a flag of an action placed at the level of its group — +// swallows the next word, and Find stops short of the real command. func checkFlagOrder(root *cobra.Command, args []string) error { - cmd, _, err := root.Find(args) - if err != nil || cmd == root { + cmd := root + last := -1 + inPath := map[int]bool{} + pendingValue := false + for i, arg := range args { + if arg == "--" { + break + } + if strings.HasPrefix(arg, "-") && arg != "-" { + name, short, inline := splitFlag(arg) + pendingValue = !inline && takesValue(cmd, name, short) + continue + } + if pendingValue { + pendingValue = false + continue + } + next := subcommand(cmd, arg) + if next == nil { + break + } + cmd, last, inPath[i] = next, i, true + } + if last < 0 { return nil } - path := strings.Fields(cmd.CommandPath())[1:] var misplaced, moved []string - i, next := 0, 0 - for ; i < len(args) && next < len(path); i++ { - arg := args[i] - if arg == path[next] { - next++ + for i, arg := range args[:last] { + if inPath[i] { continue } - if arg == "--" || !strings.HasPrefix(arg, "-") { - return nil - } - name, short, inline := splitFlag(arg) - found, takesValue := lookupFlag(cmd, name, short) - if !found { - return nil - } - misplaced = append(misplaced, strings.SplitN(arg, "=", 2)[0]) moved = append(moved, arg) - if takesValue && !inline && i+1 < len(args) { - i++ - moved = append(moved, args[i]) + if strings.HasPrefix(arg, "-") { + misplaced = append(misplaced, strings.SplitN(arg, "=", 2)[0]) } } - if len(misplaced) == 0 { + if len(moved) == 0 { return nil } + for _, flag := range misplaced { + if flag == "--version" { + return &tfserr.Error{ + Category: tfserr.Config, + Message: fmt.Sprintf("--version belongs to tfscli itself, not to %q (run it as: tfscli --version)", cmd.CommandPath()), + } + } + } + verb := "comes" if len(misplaced) > 1 { verb = "come" } - corrected := append(append(append([]string{root.Name()}, path...), moved...), args[i:]...) + corrected := append(append(strings.Fields(cmd.CommandPath()), moved...), args[last+1:]...) return &tfserr.Error{ Category: tfserr.Config, Message: fmt.Sprintf("flags must follow the command %q, and %s %s before its end (run it as: %s)", @@ -65,6 +83,16 @@ func checkFlagOrder(root *cobra.Command, args []string) error { } } +// subcommand returns the child of cmd named name, or nil. +func subcommand(cmd *cobra.Command, name string) *cobra.Command { + for _, c := range cmd.Commands() { + if c.Name() == name { + return c + } + } + return nil +} + // splitFlag takes a flag token apart: "--name", "--name=value", "-n", or // "-nvalue". inline reports whether the token carries its value. func splitFlag(arg string) (name string, short, inline bool) { @@ -79,26 +107,38 @@ func splitFlag(arg string) (name string, short, inline bool) { return body, true, false } -// lookupFlag finds a flag of cmd by name or shorthand, its own or inherited -// from a parent, and reports whether it takes a value: a flag without a value -// for its bare form, such as --project, consumes the next argument, while a -// boolean such as --verbose does not. -func lookupFlag(cmd *cobra.Command, name string, short bool) (found, takesValue bool) { - local, inherited := cmd.Flags(), cmd.InheritedFlags() - f := local.Lookup(name) - if f == nil { - f = inherited.Lookup(name) +// takesValue reports whether the flag named consumes the next argument as its +// value: a flag without a value for its bare form, such as --project, does, +// while a boolean such as --verbose does not. The flag is looked up on cmd, +// among the flags it inherits, and on every command below it, since a flag of +// an action may be placed while the walk still stands on its group. --help, +// -h, and --version, which cobra registers only when it executes a command, +// take no value, and neither does a flag found nowhere. +func takesValue(cmd *cobra.Command, name string, short bool) bool { + if name == "help" || name == "version" || (short && name == "h") { + return false } - if short { - f = local.ShorthandLookup(name) + sets := []*cobra.Command{cmd} + for len(sets) > 0 { + c := sets[0] + sets = append(sets[1:], c.Commands()...) + + local, inherited := c.Flags(), c.InheritedFlags() + f := local.Lookup(name) if f == nil { - f = inherited.ShorthandLookup(name) + f = inherited.Lookup(name) + } + if short { + f = local.ShorthandLookup(name) + if f == nil { + f = inherited.ShorthandLookup(name) + } + } + if f != nil { + return f.NoOptDefVal == "" } } - if f == nil { - return false, false - } - return true, f.NoOptDefVal == "" + return false } // shellJoin joins the arguments into a command line, quoting those a shell diff --git a/internal/cli/flagorder_test.go b/internal/cli/flagorder_test.go index 2d36fc5..514d2c5 100644 --- a/internal/cli/flagorder_test.go +++ b/internal/cli/flagorder_test.go @@ -39,6 +39,51 @@ func TestFlagBeforeTheCommandPathIsRefused(t *testing.T) { args: []string{"wit", "work-items", "-pMyProject", "get", "12345"}, want: `Error [config]: flags must follow the command "tfscli wit work-items get", and -pMyProject comes before its end (run it as: tfscli wit work-items get -pMyProject 12345)` + "\n", }, + { + name: "long help flag before the path", + args: []string{"--help", "wit", "work-items", "get"}, + want: `Error [config]: flags must follow the command "tfscli wit work-items get", and --help comes before its end (run it as: tfscli wit work-items get --help)` + "\n", + }, + { + name: "short help flag before the path", + args: []string{"-h", "wit", "work-items", "get"}, + want: `Error [config]: flags must follow the command "tfscli wit work-items get", and -h comes before its end (run it as: tfscli wit work-items get -h)` + "\n", + }, + { + name: "help flag between path words", + args: []string{"wit", "--help", "work-items", "get"}, + want: `Error [config]: flags must follow the command "tfscli wit work-items get", and --help comes before its end (run it as: tfscli wit work-items get --help)` + "\n", + }, + { + name: "boolean flag of the action at the level of its group", + args: []string{"wit", "wiql", "--time-precision", "query-by-wiql", "--query", "x"}, + want: `Error [config]: flags must follow the command "tfscli wit wiql query-by-wiql", and --time-precision comes before its end (run it as: tfscli wit wiql query-by-wiql --time-precision --query x)` + "\n", + }, + { + name: "value that names a subcommand", + args: []string{"wit", "work-items", "-p", "get", "get", "12345"}, + want: `Error [config]: flags must follow the command "tfscli wit work-items get", and -p comes before its end (run it as: tfscli wit work-items get -p get 12345)` + "\n", + }, + { + name: "unknown flag", + args: []string{"--nope", "wit", "work-items", "get", "-p", "MyProject", "12345"}, + want: `Error [config]: flags must follow the command "tfscli wit work-items get", and --nope comes before its end (run it as: tfscli wit work-items get --nope -p MyProject 12345)` + "\n", + }, + { + name: "flag before help", + args: []string{"--verbose", "help", "wit"}, + want: `Error [config]: flags must follow the command "tfscli help", and --verbose comes before its end (run it as: tfscli help --verbose wit)` + "\n", + }, + { + name: "flag before completion", + args: []string{"--verbose", "completion", "powershell"}, + want: `Error [config]: flags must follow the command "tfscli completion powershell", and --verbose comes before its end (run it as: tfscli completion powershell --verbose)` + "\n", + }, + { + name: "version flag before a path", + args: []string{"--version", "wit"}, + want: `Error [config]: --version belongs to tfscli itself, not to "tfscli wit" (run it as: tfscli --version)` + "\n", + }, } for _, tt := range tests { @@ -63,22 +108,21 @@ func TestFlagBeforeTheCommandPathIsRefused(t *testing.T) { } } -// The flags cobra handles itself and its built-in commands keep working -// wherever they are placed, since they send no request. -func TestHelpAndVersionAreNotRefusedBeforeThePath(t *testing.T) { +// Help, the version, and cobra's own commands work where the rule puts their +// flags: after the command path, or with no path at all. +func TestHelpAndVersionAfterThePath(t *testing.T) { tests := []struct { args []string want string }{ {args: []string{"--version"}, want: "tfscli dev (unknown)"}, {args: []string{"--help"}, want: "Available Commands:"}, - {args: []string{"-h", "wit"}, want: "Available Commands:"}, + {args: []string{"-h"}, want: "Available Commands:"}, {args: []string{"wit", "--help"}, want: "Available Commands:"}, {args: []string{"wit", "work-items", "--help"}, want: "Available Commands:"}, {args: []string{"wit", "work-items", "get", "--help"}, want: "--fields"}, - {args: []string{"wit", "--help", "work-items", "get"}, want: "Available Commands:"}, + {args: []string{"wit", "wiql", "query-by-wiql", "-h"}, want: "--time-precision"}, {args: []string{"help", "wit", "work-items", "get"}, want: "--fields"}, - {args: []string{"--verbose", "help", "wit"}, want: "Available Commands:"}, {args: []string{"completion", "powershell"}, want: "Register-ArgumentCompleter"}, } @@ -95,16 +139,3 @@ func TestHelpAndVersionAreNotRefusedBeforeThePath(t *testing.T) { }) } } - -// A flag the target command does not define is reported by cobra as unknown, -// not as misplaced: moving it would not make the command valid. -func TestUnknownFlagBeforeThePathIsLeftToCobra(t *testing.T) { - _, stderr, code := execute(t, nil, "--nope=1", "wit", "work-items", "get", "-p", "MyProject", "12345") - - if code != 1 { - t.Errorf("exit code = %d, want 1", code) - } - if want := "Error [config]: unknown flag: --nope"; !strings.HasPrefix(stderr, want) { - t.Errorf("stderr = %q, want it to start with %q", stderr, want) - } -} From e542e3b3a114c50e3a809cd59aac939dc9a39d13 Mon Sep 17 00:00:00 2001 From: Dmitry Pleshakov Date: Thu, 8 Oct 2026 10:04:31 +0300 Subject: [PATCH 08/25] Drop the exceptions from the flag order rule in the docs (TASK-02) With the command path found by walking the arguments, --help follows the path like any other flag, and help and completion are ordinary commands, so the brief, the README, and the agent skill no longer list exceptions; only an invocation without a path, such as tfscli --help, has none. The architecture describes the walk and why cobra's Find is not used. The changelog records the --help case and, as a fix, the false success of a flag placed before its action. Closes docs/archive/2026-10-08-tasks-command-path-walk.md Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01WCREyMqPZyHxuCRoy5FZqf --- CHANGELOG.md | 5 ++++- README.md | 2 +- docs/architecture.md | 2 +- docs/{ => archive}/2026-10-08-tasks-command-path-walk.md | 4 ++-- docs/project-brief.md | 2 +- skills/tfscli/SKILL.md | 7 ++++--- 6 files changed, 13 insertions(+), 9 deletions(-) rename docs/{ => archive}/2026-10-08-tasks-command-path-walk.md (99%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c6c8eb..51eb73e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Changed -- A flag placed before the end of the command path, such as `--verbose` in `tfscli --verbose wit work-items get 12345`, is now refused with a `config` error that shows the command with the flag moved after it, where it used to be accepted; flags now always follow the command, so that agent permission rules can match the command by its name. +- A flag placed before the end of the command path, such as `--verbose` in `tfscli --verbose wit work-items get 12345` or `--help` in `tfscli --help wit work-items get`, is now refused with a `config` error that shows the command with the flag moved after it, where it used to be accepted or to fail with a misleading "unknown command"; flags now always follow the command, so that agent permission rules can match the command by its name. + +### Fixed +- A flag of an action placed before the action, as in `tfscli wit wiql --time-precision query-by-wiql`, no longer makes tfscli print the help of the group and exit successfully without running the query. ## [0.1.1] - 2026-10-06 diff --git a/README.md b/README.md index 552d18f..7494b60 100644 --- a/README.md +++ b/README.md @@ -261,7 +261,7 @@ tfscli [flags] [arguments] Commands are named after the REST API reference: the area and the resource are the segments of the operation's page path, so Get Work Item, documented under `.../wit/work-items/get-work-item`, is `wit work-items get`. `auth login` and `licenses` are local to tfscli and have no counterpart in the API. -Flags follow the command: `tfscli wit work-items get --verbose -p MyProject 12345`, never `tfscli --verbose wit work-items get ...`. A flag placed before the last word of the command is refused with a `config` error that shows the command with the flag moved, and nothing is sent to the server. `--help` and `--version` are the exceptions. The rule keeps the command at the start of the command line, where agent permission rules look for it (see "Permission rules"). +Flags follow the command: `tfscli wit work-items get --verbose -p MyProject 12345`, never `tfscli --verbose wit work-items get ...`. A flag placed before the last word of the command is refused with a `config` error that shows the command with the flag moved, and nothing is sent to the server. `--help` is no exception: `tfscli wit work-items get --help`, not `tfscli --help wit work-items get`. The rule keeps the command at the start of the command line, where agent permission rules look for it (see "Permission rules"). `tfscli --version` prints the version and the commit it was built from; `tfscli --help`, and `--help` on any command, lists the flags. `tfscli licenses` prints the components built into the binary — tfscli itself, the Go standard library, and the third-party modules — followed by the license text of each. diff --git a/docs/architecture.md b/docs/architecture.md index 7a65f2f..e33d2d4 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -195,7 +195,7 @@ This section describes the conceptual module structure, their responsibilities, ### Modules and responsibilities -- **cli** — cobra commands, nested as area, resource, and action after the REST API reference (`wit` → `work-items` → `get`); persistent flags (`--verbose`, config overrides); orchestrates the chain config → apiclient → domain → printer → exit. Owns the markdown printer functions (one per resource and shape, such as a single work item and several) plus small per-kind scalar formatters for identity and datetime values used by those printers. No `Renderer` interface in v1 — printers are plain functions. When `--json` is added later, the interface and a second renderer can be introduced here without touching the domain layer. Enforces the command line shape that "The command line can be governed by permission rules" in `project-brief.md` requires. Cobra accepts flags at any position, so `run` checks the arguments before executing the tree: it locates the target command with `root.Find`, and a flag token placed before the last word of the command path fails with a `config` error that shows the corrected command. The check is explicit rather than a root `PersistentPreRunE`, because cobra runs only the nearest such hook and a child command defining its own would silently drop it. `--help`, `-h`, and `--version` before the end of the path, a flag the target command does not define, and the `--` terminator skip the check, leaving them to cobra's own handling. A test walks the whole command tree and fails on any command alias or on cobra's prefix matching, so that each command keeps exactly one form. +- **cli** — cobra commands, nested as area, resource, and action after the REST API reference (`wit` → `work-items` → `get`); persistent flags (`--verbose`, config overrides); orchestrates the chain config → apiclient → domain → printer → exit. Owns the markdown printer functions (one per resource and shape, such as a single work item and several) plus small per-kind scalar formatters for identity and datetime values used by those printers. No `Renderer` interface in v1 — printers are plain functions. When `--json` is added later, the interface and a second renderer can be introduced here without touching the domain layer. Enforces the command line shape that "The command line can be governed by permission rules" in `project-brief.md` requires. Cobra accepts flags at any position, so `run` checks the arguments before executing the tree: it walks the arguments from the root to find the command path, and a flag token placed before the last word of that path fails with a `config` error that shows the corrected command. The walk does not use cobra's `Find`: `Find` decides whether a flag takes a value only from the flags of the command it stands on, so a flag unknown there — `--help` before cobra registers it on execution, or a flag of an action placed at the level of its group — swallows the next word and `Find` stops short of the real command. The walk looks a flag up on the current command and on every command below it, treats `--help`, `-h`, and `--version` as taking no value, and takes a word that follows a flag with a value as that value even when it names a subcommand, as pflag would. `newRoot` registers cobra's `help` and `completion` commands up front so that the walk treats them like any other command. `--version`, defined on the root alone, is refused before a path with a message that names `tfscli --version`. The check is explicit rather than a root `PersistentPreRunE`, because cobra runs only the nearest such hook and a child command defining its own would silently drop it. A test walks the whole command tree and fails on any command alias or on cobra's prefix matching, so that each command keeps exactly one form. - **config** — resolves the credential (`URL`, `Collection`, and `PAT`; a PAT is issued for one collection) from `TFSCLI_AUTH` or `$XDG_DATA_HOME/tfscli/auth.json`, never from a mix of sources; loads the optional `$XDG_CONFIG_HOME/tfscli/config.json`, applies environment overrides (`TFSCLI_*`), applies cobra flag overrides; validates that all required fields are set for the command being run. Writes `auth.json` for `auth login` (mode `0600`, directory `0700`). Holds `URL`, `Collection`, `PAT`, `Project`, `APIVersion`, `InsecureSkipVerify`, `CABundle`. Never logs PAT. - **apiclient** — wraps `net/http`. Builds URLs from `Config.URL + Collection + path` and adds `api-version`. A configured `APIVersion` is sent as it is. Otherwise the version is negotiated per request, as the official SDKs do: `OPTIONS {URL}/{Collection}/_apis` lists every resource location with its versions, the caller names its location by id, and the request carries the location's released version, or `{maxVersion}-preview.{resourceVersion}` when it has none — the version the server chooses for a request without one. The response is not cached. A 401 or a transport failure on `OPTIONS` fails the request; any other failure sends it without a version and notes why through `Logger.Info`, and a refusal of that request for the missing version gets the settings to use in its message. A call without a location, the login check, neither negotiates nor sends a version. Sets `Authorization: Basic base64(":")` on every request. Configures TLS using `CABundle` (appended to system root pool) and `InsecureSkipVerify`. Hooks the logger via a `RoundTripper` — that transport is the single call site of `LogRequest`, so every outgoing request is logged exactly once and no other module logs HTTP traffic. Classifies HTTP outcomes into the stable error categories defined in the brief (`auth`, `not_found`, `forbidden`, `server`, `config`, `network`). - **workitem** — domain logic for Work Items: Get Work Item, Work Items - List, and Get Work Items Batch. Owns the `WorkItem`/`Field` types, the `BatchRequest`/`Batch` types for reading several work items, and the allowlist of fields known to contain HTML (`System.Description`, `Microsoft.VSTS.TCM.ReproSteps`, `Microsoft.VSTS.TCM.SystemInfo`, `Microsoft.VSTS.Common.AcceptanceCriteria`, …). After unmarshalling, tags each field with its `FieldKind`. For several work items, keeps the server's order and reports the requested ids the server did not return. Returns raw values — does not perform markdown conversion. diff --git a/docs/2026-10-08-tasks-command-path-walk.md b/docs/archive/2026-10-08-tasks-command-path-walk.md similarity index 99% rename from docs/2026-10-08-tasks-command-path-walk.md rename to docs/archive/2026-10-08-tasks-command-path-walk.md index ff67184..2267161 100644 --- a/docs/2026-10-08-tasks-command-path-walk.md +++ b/docs/archive/2026-10-08-tasks-command-path-walk.md @@ -1,6 +1,6 @@ # 2026-10-08-tasks-command-path-walk.md -**Status:** Active +**Status:** Archived ## Context @@ -74,4 +74,4 @@ match. **Definition of done:** No document states that `--help` may precede the end of the path, or that `help` and `completion` are exceptions; the architecture describes the walk instead of `Find`. `make check` passes. -**Status:** Pending +**Status:** Done diff --git a/docs/project-brief.md b/docs/project-brief.md index 1d1998f..1e8c050 100644 --- a/docs/project-brief.md +++ b/docs/project-brief.md @@ -61,7 +61,7 @@ An AI agent can call `tfscli`, read work item content, and use it meaningfully i - A path parameter that identifies the resource is a positional argument: `wit work-items get 12345` for `_apis/wit/workitems/{id}`. The project, the team, and the collection, which scope the request rather than identify the resource, are not: the collection comes from the credential, the project from `-p` or its defaults, and the team from `--team`. - A query or body parameter is a flag with the API name in kebab-case and without a leading `$`: `ids` is `--ids`, `errorPolicy` is `--error-policy`, `$expand` is `--expand`. A flag that is not given is not sent, so the server's default applies. - **The command line can be governed by permission rules.** AI agent hosts, such as Claude Code and opencode, decide whether an agent may run a command by matching its text against allow, ask, and deny rules. tfscli keeps that text predictable: - - The command path opens the command line, and flags follow its last word: `tfscli wit work-items get --verbose 12345`, not `tfscli --verbose wit work-items get 12345`. A flag placed earlier is refused with a `config` error that shows the corrected command. The exceptions are `--help` and `--version`, which may follow any word of the path, and the built-in `help` and `completion` commands. + - The command path opens the command line, and flags follow its last word: `tfscli wit work-items get --verbose 12345`, not `tfscli --verbose wit work-items get 12345`. A flag placed earlier is refused with a `config` error that shows the corrected command. The rule has no exceptions: `--help` follows the path like any other flag, and the built-in `help` and `completion` commands are commands like any other. Only an invocation without a command path, such as `tfscli --help` or `tfscli --version`, has no path for a flag to precede; `--version` belongs to the root alone. - Each command has exactly one form: no command aliases, no abbreviated command or flag names, no prefix matching of command names. - The names of local commands (`auth`, `licenses`, `help`, `completion`) never coincide with an area of the REST API reference. - Whether a command changes server state is a property of the command, never of its flags: a reading command does not gain a side effect through a flag. There is no generic pass-through command that sends an arbitrary request; should one be added, it falls under the gate flags described in "Non-Goals". diff --git a/skills/tfscli/SKILL.md b/skills/tfscli/SKILL.md index 1591a1f..96e41c4 100644 --- a/skills/tfscli/SKILL.md +++ b/skills/tfscli/SKILL.md @@ -134,9 +134,10 @@ so `-p` can carry everything a call needs beyond the credential. Write the full command name first and every flag after it: `tfscli wit work-items get --verbose -p `. A flag before the last word of the command, as in `tfscli --verbose wit work-items get ...`, is refused -with a `config` error that shows the corrected command; the user's permission -rules match the command by its name, so keep that order rather than relying on -the error. +with a `config` error that shows the corrected command, and so is `--help` in +that place: ask for help as `tfscli wit work-items get --help`. The user's +permission rules match the command by its name, so keep that order rather than +relying on the error. ### One work item From 0b2820702f0943342546a43f293299bcd5b9915b Mon Sep 17 00:00:00 2001 From: Dmitry Pleshakov Date: Fri, 9 Oct 2026 08:33:29 +0300 Subject: [PATCH 09/25] Cross-check the command path with cobra's Find (TASK-01) The walk added in the previous change is a second parser of the command line, and it diverged from cobra's in edge cases a review found. "tfscli wit work-items --fields -p get 5" passed the check and sent the request: the walk took the value -p for a new flag and stopped at work-items. It also stopped at words cobra skips ("" and "-") and at a word cobra takes as the value of a flag it does not know ("--help X"), while cobra went on to run the command behind them. The walk now reads a pending value before anything else, as pflag does, and the check locates the command both by the walk and by Find and holds to the deeper of the two: each catches the other stopping early. The command passes only when its path occupies exactly the first words. The message no longer lists the misplaced flags, since a stray word is not a flag and an attached short value made the list inaccurate; the corrected command shows what moves. A flag the command does not define is reported as unknown in pflag's wording, rather than moved into a command that would fail the same way. The corrected command is quoted for the shell in single quotes, which bash and PowerShell read alike. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01XA4Hx4kZtgobXtNCEpuiTC --- docs/2026-10-09-tasks-flag-order-review.md | 77 ++++++++ internal/cli/flagorder.go | 204 +++++++++++++-------- internal/cli/flagorder_test.go | 81 ++++++-- 3 files changed, 274 insertions(+), 88 deletions(-) create mode 100644 docs/2026-10-09-tasks-flag-order-review.md diff --git a/docs/2026-10-09-tasks-flag-order-review.md b/docs/2026-10-09-tasks-flag-order-review.md new file mode 100644 index 0000000..7213a96 --- /dev/null +++ b/docs/2026-10-09-tasks-flag-order-review.md @@ -0,0 +1,77 @@ +# 2026-10-09-tasks-flag-order-review.md + +**Status:** Active + +## Context + +A code review of the branch `command-shape-for-agent-permissions` found that +the flag order check (`internal/cli/flagorder.go`) can still be bypassed, and +that the documents promise more than the permission rules give. Confirmed on +the built binary against an unreachable server: + +| Invocation | Result | +|---|---| +| `tfscli wit work-items --fields -p get 5 -p P` | passes the check; `Error [network]`, so the request was sent | +| `tfscli --help X wit work-items get` | passes the check; cobra prints the help of `get` | +| `tfscli - wit work-items get` | passes the check; `get` receives `-` as its id | +| `tfscli "" wit work-items get 5` | passes the check; `get` receives two arguments | + +The walk read a flag value starting with `-` as a new flag, and stopped at a +word cobra skips (`""`, `-`) or at a word cobra takes as the value of a flag it +does not know (`--help X`). It is a second parser of the command line, and it +diverges from cobra's in edge cases. + +Decisions already made: + +- The walk reads the word after a flag that takes a value as that value before + anything else, whatever it starts with, as pflag does. +- The command is located both by the walk and by cobra's `Find`, and the + deeper of the two is taken: the walk catches `Find` stopping at a group, and + `Find` catches the walk stopping early. The check passes only when the path + of that command occupies exactly the first words of the command line. +- The corrected command is the path followed by the remaining words in their + original order. The message names the command and shows the corrected + command; it no longer lists the misplaced flags, since a stray word such as + `""` is not a flag and an attached short value made the list inaccurate. +- A flag before the path that the target command does not define is reported + as unknown, in cobra's wording, instead of being moved into a corrected + command that would fail the same way. The words before the path are read by + pflag's rules, so a value such as `-p` after `--fields` is not taken for a + flag. +- The corrected command quotes every word outside a safe set of characters in + single quotes, which bash and PowerShell read alike, and falls back to + double quotes only for a word containing a single quote. +- The README states that a command matching no rule gets the host's default + (Claude Code asks; opencode allows unless a catch-all `"*": "ask"` is set), + so the deny rule guards the usual form of the call and is not a security + boundary. Its examples add an ask rule on `--allow-changes`, so that future + write commands under `tfscli wit *` are not allowed by the same rule. + +--- + +### TASK-01 `cross-check` +**Description:** Implement the decisions above in `internal/cli/flagorder.go`: +read a pending value before a flag, cross-check the walk with `Find`, build the +corrected command from the remaining words, report unknown flags before the +path as unknown, quote the corrected command for the shell, and make the long +and short lookups in `takesValue` exclusive branches. In +`internal/cli/flagorder_test.go`, add the four invocations of the table, an +unknown flag before the path, a value that starts with `-`, and quoting of a +word with `&` and `[`; check that stderr is empty on the accepted invocations. +**Definition of done:** Every invocation of the table is refused before any +request; the tests listed pass; `make check` passes. +**Status:** Done + +### TASK-02 `docs` +**Description:** In `README.md`: describe the fallback to the host's default +for an unmatched command and its consequence for the deny rule; add the ask +rule on `--allow-changes` to the Claude Code and opencode examples; state that +every command starts with its full name without the area/resource/action +template; note the separate message for `--version` before a command. In +`CONTRIBUTING.md`, add the command line rules and the gate flags to the list +in "Before changing behaviour", and the gate flags to the precedence item; do +the same for the precedence item in `CLAUDE.md`. In `docs/architecture.md`, +describe the cross-check with `Find`, unknown flags, and quoting. +**Definition of done:** The documents agree with the code and with each other +on the points above. `make check` passes. +**Status:** Pending diff --git a/internal/cli/flagorder.go b/internal/cli/flagorder.go index 0762bde..19e903b 100644 --- a/internal/cli/flagorder.go +++ b/internal/cli/flagorder.go @@ -2,6 +2,7 @@ package cli import ( "fmt" + "slices" "strconv" "strings" @@ -10,77 +11,121 @@ import ( "github.com/dpleshakov/tfscli/internal/tfserr" ) -// checkFlagOrder refuses a flag placed before the last word of the command -// path, such as --verbose in "tfscli --verbose wit work-items get 1". Agent +// checkFlagOrder refuses a command line whose command path does not occupy its +// first words, such as "tfscli --verbose wit work-items get 1". Agent // permission rules match the command text by its prefix, so the command path // has to open the command line; cobra itself accepts flags at any position. // -// The path is found by walking the arguments from the root rather than with -// cobra's Find. Find decides whether a flag takes a value only from the flags -// of the command it stands on, so a flag unknown there — --help before cobra -// registers it, or a flag of an action placed at the level of its group — -// swallows the next word, and Find stops short of the real command. +// The command is located twice, and the deeper result is taken. Cobra's Find +// decides whether a flag takes a value only from the flags of the command it +// stands on, so a flag unknown there — a flag of an action placed at the level +// of its group — swallows the next word and Find stops at the group. The walk +// in walkPath looks flags up in the whole subtree instead, but it can stop +// where Find goes on: at a word Find skips, such as "" or "-", or at a word +// Find takes as the value of a flag it does not know, such as X in +// "--help X". Whichever command is deeper is the one cobra may run, and it is +// what the check holds to. func checkFlagOrder(root *cobra.Command, args []string) error { - cmd := root - last := -1 - inPath := map[int]bool{} - pendingValue := false - for i, arg := range args { - if arg == "--" { - break - } - if strings.HasPrefix(arg, "-") && arg != "-" { - name, short, inline := splitFlag(arg) - pendingValue = !inline && takesValue(cmd, name, short) - continue - } - if pendingValue { - pendingValue = false - continue - } - next := subcommand(cmd, arg) - if next == nil { - break - } - cmd, last, inPath[i] = next, i, true + target := walkPath(root, args) + if found, _, err := root.Find(args); err == nil && depth(found) > depth(target) { + target = found } - if last < 0 { + path := strings.Fields(target.CommandPath())[1:] + if len(path) == 0 || slices.Equal(args[:min(len(path), len(args))], path) { return nil } - var misplaced, moved []string - for i, arg := range args[:last] { - if inPath[i] { + // The corrected command is the path followed by every other word in its + // original order; before is the part of it that preceded the path. + var before, rest []string + next := 0 + for _, arg := range args { + if next < len(path) && arg == path[next] { + next++ continue } - moved = append(moved, arg) - if strings.HasPrefix(arg, "-") { - misplaced = append(misplaced, strings.SplitN(arg, "=", 2)[0]) + if next < len(path) { + before = append(before, arg) } + rest = append(rest, arg) } - if len(moved) == 0 { - return nil + + if slices.Contains(before, "--version") { + return &tfserr.Error{ + Category: tfserr.Config, + Message: fmt.Sprintf("--version belongs to tfscli itself, not to %q (run it as: tfscli --version)", target.CommandPath()), + } + } + if err := unknownFlag(target, before); err != nil { + return err + } + return &tfserr.Error{ + Category: tfserr.Config, + Message: fmt.Sprintf("the command %q must come first, before its flags and arguments (run it as: %s)", + target.CommandPath(), shellJoin(append(append([]string{root.Name()}, path...), rest...))), } +} - for _, flag := range misplaced { - if flag == "--version" { - return &tfserr.Error{ - Category: tfserr.Config, - Message: fmt.Sprintf("--version belongs to tfscli itself, not to %q (run it as: tfscli --version)", cmd.CommandPath()), +// walkPath follows the command path through the arguments from the root and +// returns the deepest command it reaches. A word that follows a flag taking a +// value is that value, whatever it starts with or names, as pflag reads it. +func walkPath(root *cobra.Command, args []string) *cobra.Command { + cmd := root + pendingValue := false + for _, arg := range args { + switch { + case arg == "--": + return cmd + case pendingValue: + pendingValue = false + case strings.HasPrefix(arg, "-") && arg != "-": + name, short, inline := splitFlag(arg) + pendingValue = !inline && takesValue(cmd, name, short) + default: + next := subcommand(cmd, arg) + if next == nil { + return cmd } + cmd = next } } + return cmd +} - verb := "comes" - if len(misplaced) > 1 { - verb = "come" +// depth is the number of words in the command path of cmd below the root. +func depth(cmd *cobra.Command) int { + n := 0 + for ; cmd.HasParent(); cmd = cmd.Parent() { + n++ } - corrected := append(append(strings.Fields(cmd.CommandPath()), moved...), args[last+1:]...) - return &tfserr.Error{ - Category: tfserr.Config, - Message: fmt.Sprintf("flags must follow the command %q, and %s %s before its end (run it as: %s)", - cmd.CommandPath(), strings.Join(misplaced, ", "), verb, shellJoin(corrected)), + return n +} + +// unknownFlag reports the first flag among words, read as pflag reads them, +// that cmd does not define, in pflag's own wording. Moving such a flag after +// the path would not make the command valid, so it is reported as unknown +// rather than as misplaced. +func unknownFlag(cmd *cobra.Command, words []string) error { + for i := 0; i < len(words); i++ { + word := words[i] + if word == "-" || !strings.HasPrefix(word, "-") { + continue + } + name, short, inline := splitFlag(word) + if name == "help" && !short || name == "h" && short { + continue + } + found, value := lookupFlag(cmd, name, short) + switch { + case !found && short: + return &tfserr.Error{Category: tfserr.Config, Message: fmt.Sprintf("unknown shorthand flag: '%s' in %s", name, word)} + case !found: + return &tfserr.Error{Category: tfserr.Config, Message: "unknown flag: --" + name} + case value && !inline: + i++ + } } + return nil } // subcommand returns the child of cmd named name, or nil. @@ -118,38 +163,53 @@ func takesValue(cmd *cobra.Command, name string, short bool) bool { if name == "help" || name == "version" || (short && name == "h") { return false } - sets := []*cobra.Command{cmd} - for len(sets) > 0 { - c := sets[0] - sets = append(sets[1:], c.Commands()...) - - local, inherited := c.Flags(), c.InheritedFlags() - f := local.Lookup(name) - if f == nil { - f = inherited.Lookup(name) - } - if short { - f = local.ShorthandLookup(name) - if f == nil { - f = inherited.ShorthandLookup(name) - } - } - if f != nil { - return f.NoOptDefVal == "" + queue := []*cobra.Command{cmd} + for len(queue) > 0 { + c := queue[0] + queue = append(queue[1:], c.Commands()...) + if found, value := lookupFlag(c, name, short); found { + return value } } return false } -// shellJoin joins the arguments into a command line, quoting those a shell -// would split or drop. +// lookupFlag finds a flag of cmd, its own or inherited, by name or by +// shorthand, and reports whether it takes a value. +func lookupFlag(cmd *cobra.Command, name string, short bool) (found, takesValue bool) { + local, inherited := cmd.Flags(), cmd.InheritedFlags() + lookup, fallback := local.Lookup, inherited.Lookup + if short { + lookup, fallback = local.ShorthandLookup, inherited.ShorthandLookup + } + f := lookup(name) + if f == nil { + f = fallback(name) + } + if f == nil { + return false, false + } + return true, f.NoOptDefVal == "" +} + +// shellJoin joins the arguments into a command line. A word with a character +// outside a safe set is put in single quotes, which bash and PowerShell read +// alike; a word that itself contains a single quote is put in double quotes +// instead. func shellJoin(args []string) string { quoted := make([]string, len(args)) for i, a := range args { - if a == "" || strings.ContainsAny(a, " \t\"'") { + switch { + case a != "" && strings.Trim(a, shellSafe) == "": + case !strings.Contains(a, "'"): + a = "'" + a + "'" + default: a = strconv.Quote(a) } quoted[i] = a } return strings.Join(quoted, " ") } + +// shellSafe holds the characters that need no quoting in a shell word. +const shellSafe = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_@%+=:,./-" diff --git a/internal/cli/flagorder_test.go b/internal/cli/flagorder_test.go index 514d2c5..f03ab6d 100644 --- a/internal/cli/flagorder_test.go +++ b/internal/cli/flagorder_test.go @@ -6,7 +6,13 @@ import ( "testing" ) -// A flag before the end of the command path would slip past an agent +// refused is the error for a command line whose command path does not come +// first. +func refused(command, corrected string) string { + return `Error [config]: the command "` + command + `" must come first, before its flags and arguments (run it as: ` + corrected + ")\n" +} + +// A command path that does not open the command line would slip past an agent // permission rule written as a prefix, so it is refused before any request. func TestFlagBeforeTheCommandPathIsRefused(t *testing.T) { tests := []struct { @@ -17,73 +23,113 @@ func TestFlagBeforeTheCommandPathIsRefused(t *testing.T) { { name: "persistent flag at the root", args: []string{"--verbose", "wit", "work-items", "get", "-p", "MyProject", "12345"}, - want: `Error [config]: flags must follow the command "tfscli wit work-items get", and --verbose comes before its end (run it as: tfscli wit work-items get --verbose -p MyProject 12345)` + "\n", + want: refused("tfscli wit work-items get", "tfscli wit work-items get --verbose -p MyProject 12345"), }, { name: "flag with a separate value between path words", args: []string{"wit", "--api-version", "7.1", "work-items", "get", "-p", "MyProject", "12345"}, - want: `Error [config]: flags must follow the command "tfscli wit work-items get", and --api-version comes before its end (run it as: tfscli wit work-items get --api-version 7.1 -p MyProject 12345)` + "\n", + want: refused("tfscli wit work-items get", "tfscli wit work-items get --api-version 7.1 -p MyProject 12345"), }, { name: "flag with an inline value and a quoted argument", args: []string{"--api-version=7.1", "wit", "wiql", "query-by-wiql", "--query", "SELECT [System.Id] FROM WorkItems"}, - want: `Error [config]: flags must follow the command "tfscli wit wiql query-by-wiql", and --api-version comes before its end (run it as: tfscli wit wiql query-by-wiql --api-version=7.1 --query "SELECT [System.Id] FROM WorkItems")` + "\n", + want: refused("tfscli wit wiql query-by-wiql", "tfscli wit wiql query-by-wiql --api-version=7.1 --query 'SELECT [System.Id] FROM WorkItems'"), }, { name: "flags of the action before its name", args: []string{"wit", "work-items", "-p", "MyProject", "--verbose", "get", "12345"}, - want: `Error [config]: flags must follow the command "tfscli wit work-items get", and -p, --verbose come before its end (run it as: tfscli wit work-items get -p MyProject --verbose 12345)` + "\n", + want: refused("tfscli wit work-items get", "tfscli wit work-items get -p MyProject --verbose 12345"), }, { name: "shorthand with an attached value", args: []string{"wit", "work-items", "-pMyProject", "get", "12345"}, - want: `Error [config]: flags must follow the command "tfscli wit work-items get", and -pMyProject comes before its end (run it as: tfscli wit work-items get -pMyProject 12345)` + "\n", + want: refused("tfscli wit work-items get", "tfscli wit work-items get -pMyProject 12345"), }, { name: "long help flag before the path", args: []string{"--help", "wit", "work-items", "get"}, - want: `Error [config]: flags must follow the command "tfscli wit work-items get", and --help comes before its end (run it as: tfscli wit work-items get --help)` + "\n", + want: refused("tfscli wit work-items get", "tfscli wit work-items get --help"), }, { name: "short help flag before the path", args: []string{"-h", "wit", "work-items", "get"}, - want: `Error [config]: flags must follow the command "tfscli wit work-items get", and -h comes before its end (run it as: tfscli wit work-items get -h)` + "\n", + want: refused("tfscli wit work-items get", "tfscli wit work-items get -h"), }, { name: "help flag between path words", args: []string{"wit", "--help", "work-items", "get"}, - want: `Error [config]: flags must follow the command "tfscli wit work-items get", and --help comes before its end (run it as: tfscli wit work-items get --help)` + "\n", + want: refused("tfscli wit work-items get", "tfscli wit work-items get --help"), + }, + { + name: "help flag followed by a stray word", + args: []string{"--help", "X", "wit", "work-items", "get"}, + want: refused("tfscli wit work-items get", "tfscli wit work-items get --help X"), }, { name: "boolean flag of the action at the level of its group", args: []string{"wit", "wiql", "--time-precision", "query-by-wiql", "--query", "x"}, - want: `Error [config]: flags must follow the command "tfscli wit wiql query-by-wiql", and --time-precision comes before its end (run it as: tfscli wit wiql query-by-wiql --time-precision --query x)` + "\n", + want: refused("tfscli wit wiql query-by-wiql", "tfscli wit wiql query-by-wiql --time-precision --query x"), }, { name: "value that names a subcommand", args: []string{"wit", "work-items", "-p", "get", "get", "12345"}, - want: `Error [config]: flags must follow the command "tfscli wit work-items get", and -p comes before its end (run it as: tfscli wit work-items get -p get 12345)` + "\n", + want: refused("tfscli wit work-items get", "tfscli wit work-items get -p get 12345"), }, { - name: "unknown flag", - args: []string{"--nope", "wit", "work-items", "get", "-p", "MyProject", "12345"}, - want: `Error [config]: flags must follow the command "tfscli wit work-items get", and --nope comes before its end (run it as: tfscli wit work-items get --nope -p MyProject 12345)` + "\n", + name: "value that starts with a dash", + args: []string{"wit", "work-items", "--fields", "-p", "get", "5", "-p", "MyProject"}, + want: refused("tfscli wit work-items get", "tfscli wit work-items get --fields -p 5 -p MyProject"), + }, + { + name: "lone dash before the path", + args: []string{"-", "wit", "work-items", "get"}, + want: refused("tfscli wit work-items get", "tfscli wit work-items get -"), + }, + { + name: "empty word before the path", + args: []string{"", "wit", "work-items", "get", "5"}, + want: refused("tfscli wit work-items get", "tfscli wit work-items get '' 5"), + }, + { + name: "word a shell would interpret", + args: []string{"--verbose", "wit", "wiql", "query-by-wiql", "--team", "R&D", "--query", "x"}, + want: refused("tfscli wit wiql query-by-wiql", "tfscli wit wiql query-by-wiql --verbose --team 'R&D' --query x"), + }, + { + name: "word with a single quote", + args: []string{"--verbose", "wit", "wiql", "query-by-wiql", "--query", "it's"}, + want: refused("tfscli wit wiql query-by-wiql", `tfscli wit wiql query-by-wiql --verbose --query "it's"`), }, { name: "flag before help", args: []string{"--verbose", "help", "wit"}, - want: `Error [config]: flags must follow the command "tfscli help", and --verbose comes before its end (run it as: tfscli help --verbose wit)` + "\n", + want: refused("tfscli help", "tfscli help --verbose wit"), }, { name: "flag before completion", args: []string{"--verbose", "completion", "powershell"}, - want: `Error [config]: flags must follow the command "tfscli completion powershell", and --verbose comes before its end (run it as: tfscli completion powershell --verbose)` + "\n", + want: refused("tfscli completion powershell", "tfscli completion powershell --verbose"), }, { name: "version flag before a path", args: []string{"--version", "wit"}, want: `Error [config]: --version belongs to tfscli itself, not to "tfscli wit" (run it as: tfscli --version)` + "\n", }, + { + name: "unknown flag before the path", + args: []string{"--nope", "wit", "work-items", "get", "-p", "MyProject", "12345"}, + want: "Error [config]: unknown flag: --nope\n", + }, + { + name: "unknown flag swallowing a stray word", + args: []string{"--nope", "X", "wit", "work-items", "get", "-p", "MyProject", "12345"}, + want: "Error [config]: unknown flag: --nope\n", + }, + { + name: "unknown shorthand before the path", + args: []string{"-z", "wit", "work-items", "get", "-p", "MyProject", "12345"}, + want: "Error [config]: unknown shorthand flag: 'z' in -z\n", + }, } for _, tt := range tests { @@ -133,6 +179,9 @@ func TestHelpAndVersionAfterThePath(t *testing.T) { if code != 0 { t.Fatalf("exit code = %d, want 0 (stderr: %s)", code, stderr) } + if stderr != "" { + t.Errorf("stderr = %q, want nothing", stderr) + } if !strings.Contains(stdout, tt.want) { t.Errorf("stdout does not contain %q:\n%s", tt.want, stdout) } From ee2e3d4d525cc8811bdbc4dc823663d9ca8d2440 Mon Sep 17 00:00:00 2001 From: Dmitry Pleshakov Date: Fri, 9 Oct 2026 08:34:29 +0300 Subject: [PATCH 10/25] Align the docs with the cross-checked flag order rule (TASK-02) The README said a call through tfscli.exe or a full path is asked about, which holds for Claude Code but not for opencode, where a command matching no rule is allowed unless a catch-all ask is set; it now says so, and that the deny rule guards the usual form of the call rather than being a security boundary. Its examples gain an ask rule on --allow-changes, since future write commands will fall under the same "tfscli wit *" allow rule as the reading ones. CONTRIBUTING repeats the brief's constraints for contributors who do not open it, and now lists the command line rules and the gate flags as CLAUDE.md does. The architecture describes the cross-check with Find, unknown flags, and the quoting of the corrected command. Closes docs/archive/2026-10-09-tasks-flag-order-review.md Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01XA4Hx4kZtgobXtNCEpuiTC --- CLAUDE.md | 2 +- CONTRIBUTING.md | 10 ++++- README.md | 37 +++++++++++++------ docs/architecture.md | 2 +- .../2026-10-09-tasks-flag-order-review.md | 4 +- 5 files changed, 38 insertions(+), 17 deletions(-) rename docs/{ => archive}/2026-10-09-tasks-flag-order-review.md (99%) diff --git a/CLAUDE.md b/CLAUDE.md index 168f4dd..3903954 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -28,7 +28,7 @@ These come from `project-brief.md` and override casual feature requests. If a ch - **Mirror TFS REST API structure.** Command hierarchy, parameter names, and behaviour follow the API. Any deviation needs explicit usability justification — don't invent new abstractions (no custom query syntax, no synthetic batch endpoints, no caching). - **Stateless.** No daemon, no background process, no on-disk cache. Every call hits the server. - **Stable error contract.** Error categories (`auth`, `not_found`, `forbidden`, `server`, `config`, `network`) are a contract: existing categories are never removed or renamed. New ones may be added. Format: `Error [category]: message (HTTP status)` to stderr, non-zero exit. -- **Config precedence:** CLI flag > env var (`TFSCLI_PROJECT`, `TFSCLI_API_VERSION`) > config file (optional) > built-in default. The credential is outside this chain (see above). +- **Config precedence:** CLI flag > env var (`TFSCLI_PROJECT`, `TFSCLI_API_VERSION`) > config file (optional) > built-in default. The credential and the gate flags of write operations are outside this chain (see above). - **Target is on-prem TFS / Azure DevOps Server; the API version is negotiated.** Without a configured version, each request is preceded by `OPTIONS {url}/{collection}/_apis` and carries the version the server lists for its resource; when the negotiation fails, the request goes out without a version, as before. A configured version is sent as is. `auth login` never sends one. Cloud Azure DevOps Services may work incidentally but is not tested or supported. - **Do not narrow server compatibility.** A change must not stop tfscli from working with a TFS / Azure DevOps Server version it worked with before, unless that is an explicitly discussed and recorded decision (see "Server compatibility is preserved" in `project-brief.md`). - **Works out of the box; errors lead the way.** Assume the user has read nothing and configured nothing. Do not add a required setting where a working default exists, and do not guess where the answer is ambiguous — ask the user instead. Every error a first-time user can hit names the concrete next step, and says when only a person can take it (see "Works out of the box; errors lead the way" in `project-brief.md`). diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ccb487f..3c94ea0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -75,13 +75,21 @@ if a change would break one, raise it before implementing. - PAT authentication only; no SSPI or NTLM. The PAT is entered interactively by `tfscli auth login` and never accepted as a command argument. - Read-only in v1; write operations additionally depend on JSON output landing first. +- When write operations are added, every operation that changes server state requires + `--allow-changes`, and an irreversible one `--allow-irreversible` as well. Changing + server state is a property of the command, never of a flag of a reading command. The + gate flags have no short form, environment variable, or config key. - Markdown is the default output format. - The command hierarchy and parameter names mirror the TFS REST API; a deviation needs an explicit usability justification, and no query syntax or batch endpoint is invented. +- The command line stays matchable by agent permission rules: flags follow the command + path, each command has one form (no aliases, abbreviations, or prefix matching), local + command names never coincide with REST API areas, and there is no generic pass-through + command. - Stateless: no daemon, no background process, no on-disk state beyond the config file and the credential written by `auth login`. - The error categories `auth`, `not_found`, `forbidden`, `server`, `config`, and `network` are a contract. New ones may be added; existing ones are never renamed or removed. -- Configuration precedence is flag, environment variable, config file, built-in default. The server URL, the collection, and the PAT stay outside it: they come only from `auth.json` or `TFSCLI_AUTH`, together. +- Configuration precedence is flag, environment variable, config file, built-in default. The server URL, the collection, and the PAT stay outside it: they come only from `auth.json` or `TFSCLI_AUTH`, together. The gate flags of write operations stay outside it too: they are given on the command line only. **Dependencies.** Three direct dependencies — `cobra`, `html-to-markdown/v2`, and `golang.org/x/term` — are the intended total. HTTP is `net/http`; JSON and configuration diff --git a/README.md b/README.md index 7494b60..9eb0b69 100644 --- a/README.md +++ b/README.md @@ -155,14 +155,20 @@ plain markdown under a short YAML header. ### Permission rules Both agents decide whether a shell command may run by matching its text against -rules. Every tfscli command starts with its full name, `tfscli -`, followed by flags and arguments, and each command has exactly one -name, so a rule written as a prefix covers the command whatever its flags. The -current commands only read; `auth login` is interactive and is not for an -agent to run. +rules. Every tfscli command starts with its full name, such as +`tfscli wit work-items get` or `tfscli licenses`, followed by flags and +arguments, and each command has exactly one name, so a rule written as a prefix +covers the command whatever its flags. The current commands only read; +`auth login` is interactive and is not for an agent to run. + +Commands that change data are not part of tfscli yet. When they are added, +each will require the flag `--allow-changes` and will fall under `tfscli wit *` +like the reading commands, so the examples below already ask before any +command carrying that flag. Claude Code, in `.claude/settings.json` or `~/.claude/settings.json`; the -`PowerShell` rules apply where the agent runs commands through PowerShell: +`PowerShell` rules apply where the agent runs commands through PowerShell. An +`ask` rule takes precedence over an `allow` rule: ```json { @@ -173,6 +179,10 @@ Claude Code, in `.claude/settings.json` or `~/.claude/settings.json`; the "PowerShell(tfscli wit *)", "PowerShell(tfscli licenses)" ], + "ask": [ + "Bash(*--allow-changes*)", + "PowerShell(*--allow-changes*)" + ], "deny": [ "Bash(tfscli auth *)", "PowerShell(tfscli auth *)" @@ -182,7 +192,7 @@ Claude Code, in `.claude/settings.json` or `~/.claude/settings.json`; the ``` opencode, in `opencode.json`. The last matching rule wins, so place these after -any catch-all `"*"` rule: +any catch-all `"*"` rule, in this order: ```json { @@ -190,15 +200,18 @@ any catch-all `"*"` rule: "bash": { "tfscli wit *": "allow", "tfscli licenses": "allow", - "tfscli auth *": "deny" + "tfscli auth *": "deny", + "*--allow-changes*": "ask" } } } ``` -A rule matches the program name as the agent types it: a call through -`tfscli.exe` or a full path does not match `tfscli wit *` and is asked about -instead. +A rule matches the command as the agent types it. A call through `tfscli.exe` +or a full path matches none of the `tfscli` rules and gets the agent's default +for unmatched commands: Claude Code asks in its default mode, while opencode +allows it unless a catch-all `"*": "ask"` is set. The deny rule therefore keeps +an agent from the usual form of the call; it is not a security boundary. ## Configuration @@ -261,7 +274,7 @@ tfscli [flags] [arguments] Commands are named after the REST API reference: the area and the resource are the segments of the operation's page path, so Get Work Item, documented under `.../wit/work-items/get-work-item`, is `wit work-items get`. `auth login` and `licenses` are local to tfscli and have no counterpart in the API. -Flags follow the command: `tfscli wit work-items get --verbose -p MyProject 12345`, never `tfscli --verbose wit work-items get ...`. A flag placed before the last word of the command is refused with a `config` error that shows the command with the flag moved, and nothing is sent to the server. `--help` is no exception: `tfscli wit work-items get --help`, not `tfscli --help wit work-items get`. The rule keeps the command at the start of the command line, where agent permission rules look for it (see "Permission rules"). +Flags follow the command: `tfscli wit work-items get --verbose -p MyProject 12345`, never `tfscli --verbose wit work-items get ...`. A flag placed before the last word of the command is refused with a `config` error that shows the command with the flag moved, and nothing is sent to the server; a flag that the command does not define is reported as unknown instead. `--help` is no exception: `tfscli wit work-items get --help`, not `tfscli --help wit work-items get`. `--version` belongs to `tfscli` alone, and placed before a command it is refused with a message that names `tfscli --version`. The rule keeps the command at the start of the command line, where agent permission rules look for it (see "Permission rules"). `tfscli --version` prints the version and the commit it was built from; `tfscli --help`, and `--help` on any command, lists the flags. `tfscli licenses` prints the components built into the binary — tfscli itself, the Go standard library, and the third-party modules — followed by the license text of each. diff --git a/docs/architecture.md b/docs/architecture.md index e33d2d4..e461807 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -195,7 +195,7 @@ This section describes the conceptual module structure, their responsibilities, ### Modules and responsibilities -- **cli** — cobra commands, nested as area, resource, and action after the REST API reference (`wit` → `work-items` → `get`); persistent flags (`--verbose`, config overrides); orchestrates the chain config → apiclient → domain → printer → exit. Owns the markdown printer functions (one per resource and shape, such as a single work item and several) plus small per-kind scalar formatters for identity and datetime values used by those printers. No `Renderer` interface in v1 — printers are plain functions. When `--json` is added later, the interface and a second renderer can be introduced here without touching the domain layer. Enforces the command line shape that "The command line can be governed by permission rules" in `project-brief.md` requires. Cobra accepts flags at any position, so `run` checks the arguments before executing the tree: it walks the arguments from the root to find the command path, and a flag token placed before the last word of that path fails with a `config` error that shows the corrected command. The walk does not use cobra's `Find`: `Find` decides whether a flag takes a value only from the flags of the command it stands on, so a flag unknown there — `--help` before cobra registers it on execution, or a flag of an action placed at the level of its group — swallows the next word and `Find` stops short of the real command. The walk looks a flag up on the current command and on every command below it, treats `--help`, `-h`, and `--version` as taking no value, and takes a word that follows a flag with a value as that value even when it names a subcommand, as pflag would. `newRoot` registers cobra's `help` and `completion` commands up front so that the walk treats them like any other command. `--version`, defined on the root alone, is refused before a path with a message that names `tfscli --version`. The check is explicit rather than a root `PersistentPreRunE`, because cobra runs only the nearest such hook and a child command defining its own would silently drop it. A test walks the whole command tree and fails on any command alias or on cobra's prefix matching, so that each command keeps exactly one form. +- **cli** — cobra commands, nested as area, resource, and action after the REST API reference (`wit` → `work-items` → `get`); persistent flags (`--verbose`, config overrides); orchestrates the chain config → apiclient → domain → printer → exit. Owns the markdown printer functions (one per resource and shape, such as a single work item and several) plus small per-kind scalar formatters for identity and datetime values used by those printers. No `Renderer` interface in v1 — printers are plain functions. When `--json` is added later, the interface and a second renderer can be introduced here without touching the domain layer. Enforces the command line shape that "The command line can be governed by permission rules" in `project-brief.md` requires. Cobra accepts flags at any position, so `run` checks the arguments before executing the tree: the path of the command to be run must occupy exactly the first words of the command line, and otherwise the command fails with a `config` error that shows the corrected command — the path followed by every other word in its original order, quoted for the shell in single quotes, which bash and PowerShell read alike. The command is located twice, and the deeper result is taken. Cobra's `Find` decides whether a flag takes a value only from the flags of the command it stands on, so a flag of an action placed at the level of its group swallows the next word and `Find` stops at the group. The walk in `walkPath` looks a flag up on the current command and on every command below it, treats `--help`, `-h`, and `--version` as taking no value, and reads the word after a flag with a value as that value whatever it starts with or names, as pflag does; but it stops where `Find` goes on, at a word `Find` skips (`""`, `-`) or takes as the value of a flag it does not know (`--help X`). Each catches the other stopping early. `newRoot` registers cobra's `help` and `completion` commands up front so that both treat them like any other command. A flag before the path that the command does not define is reported as unknown, in pflag's wording, rather than moved into a corrected command that would fail the same way; `--version`, defined on the root alone, is refused before a path with a message that names `tfscli --version`. The check is explicit rather than a root `PersistentPreRunE`, because cobra runs only the nearest such hook and a child command defining its own would silently drop it. A test walks the whole command tree and fails on any command alias or on cobra's prefix matching, so that each command keeps exactly one form. - **config** — resolves the credential (`URL`, `Collection`, and `PAT`; a PAT is issued for one collection) from `TFSCLI_AUTH` or `$XDG_DATA_HOME/tfscli/auth.json`, never from a mix of sources; loads the optional `$XDG_CONFIG_HOME/tfscli/config.json`, applies environment overrides (`TFSCLI_*`), applies cobra flag overrides; validates that all required fields are set for the command being run. Writes `auth.json` for `auth login` (mode `0600`, directory `0700`). Holds `URL`, `Collection`, `PAT`, `Project`, `APIVersion`, `InsecureSkipVerify`, `CABundle`. Never logs PAT. - **apiclient** — wraps `net/http`. Builds URLs from `Config.URL + Collection + path` and adds `api-version`. A configured `APIVersion` is sent as it is. Otherwise the version is negotiated per request, as the official SDKs do: `OPTIONS {URL}/{Collection}/_apis` lists every resource location with its versions, the caller names its location by id, and the request carries the location's released version, or `{maxVersion}-preview.{resourceVersion}` when it has none — the version the server chooses for a request without one. The response is not cached. A 401 or a transport failure on `OPTIONS` fails the request; any other failure sends it without a version and notes why through `Logger.Info`, and a refusal of that request for the missing version gets the settings to use in its message. A call without a location, the login check, neither negotiates nor sends a version. Sets `Authorization: Basic base64(":")` on every request. Configures TLS using `CABundle` (appended to system root pool) and `InsecureSkipVerify`. Hooks the logger via a `RoundTripper` — that transport is the single call site of `LogRequest`, so every outgoing request is logged exactly once and no other module logs HTTP traffic. Classifies HTTP outcomes into the stable error categories defined in the brief (`auth`, `not_found`, `forbidden`, `server`, `config`, `network`). - **workitem** — domain logic for Work Items: Get Work Item, Work Items - List, and Get Work Items Batch. Owns the `WorkItem`/`Field` types, the `BatchRequest`/`Batch` types for reading several work items, and the allowlist of fields known to contain HTML (`System.Description`, `Microsoft.VSTS.TCM.ReproSteps`, `Microsoft.VSTS.TCM.SystemInfo`, `Microsoft.VSTS.Common.AcceptanceCriteria`, …). After unmarshalling, tags each field with its `FieldKind`. For several work items, keeps the server's order and reports the requested ids the server did not return. Returns raw values — does not perform markdown conversion. diff --git a/docs/2026-10-09-tasks-flag-order-review.md b/docs/archive/2026-10-09-tasks-flag-order-review.md similarity index 99% rename from docs/2026-10-09-tasks-flag-order-review.md rename to docs/archive/2026-10-09-tasks-flag-order-review.md index 7213a96..1f7e089 100644 --- a/docs/2026-10-09-tasks-flag-order-review.md +++ b/docs/archive/2026-10-09-tasks-flag-order-review.md @@ -1,6 +1,6 @@ # 2026-10-09-tasks-flag-order-review.md -**Status:** Active +**Status:** Archived ## Context @@ -74,4 +74,4 @@ the same for the precedence item in `CLAUDE.md`. In `docs/architecture.md`, describe the cross-check with `Find`, unknown flags, and quoting. **Definition of done:** The documents agree with the code and with each other on the points above. `make check` passes. -**Status:** Pending +**Status:** Done From fba814846a551c654c6b0f0f9288942384462f3b Mon Sep 17 00:00:00 2001 From: Dmitry Pleshakov Date: Fri, 9 Oct 2026 08:47:25 +0300 Subject: [PATCH 11/25] Find the command path without reading flag values (TASK-01) The check located the command twice, by a walk that repeated pflag's rules for flag values and by cobra's Find, and took the deeper result. Both defects found so far were divergences between that walk and pflag. The shape of the command tree makes flag values irrelevant: a command with subcommands only groups them, taking no argument and defining no flag of its own, so in a valid command line no word naming a subcommand can follow the path. The command is now the one reached by taking, in order, every word that names a subcommand of the command reached so far. That reaches at least as deep as Find, which descends only through such words, so the walk over flag values and the cross-check are gone; every case of the existing tests passes unchanged. The rule now depends on the shape of the tree, so the command tree test requires it of every group. It found at once that auth was built as a plain cobra command rather than with newGroupCmd, and that "tfscli auth bogus" printed the help with exit 0; auth is now built like the other groups and reports an unknown command. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01XA4Hx4kZtgobXtNCEpuiTC --- docs/2026-10-09-tasks-simple-path-rule.md | 65 ++++++++++++ internal/cli/auth.go | 5 +- internal/cli/cli_test.go | 5 + internal/cli/flagorder.go | 119 ++++++---------------- internal/cli/tree_test.go | 26 +++++ 5 files changed, 130 insertions(+), 90 deletions(-) create mode 100644 docs/2026-10-09-tasks-simple-path-rule.md diff --git a/docs/2026-10-09-tasks-simple-path-rule.md b/docs/2026-10-09-tasks-simple-path-rule.md new file mode 100644 index 0000000..50c344e --- /dev/null +++ b/docs/2026-10-09-tasks-simple-path-rule.md @@ -0,0 +1,65 @@ +# 2026-10-09-tasks-simple-path-rule.md + +**Status:** Active + +## Context + +The flag order check locates the command twice: by a walk that repeats +pflag's rules for flag values, and by cobra's `Find`, taking the deeper of the +two (`docs/archive/2026-10-09-tasks-flag-order-review.md`). Both defects found +so far were divergences between the walk and pflag; the cross-check covers +them, at the cost of two parsers guarding each other. + +The shape of the command tree makes knowledge of flag values unnecessary. A +command with subcommands only groups them: it takes no arguments, defines no +flags of its own, and does nothing but print its help; only a command without +subcommands runs. In a valid command line, therefore, every word that names a +subcommand of the command reached so far is a step of the path, and no such +word can follow the last one: the last step has no subcommands. + +Decisions already made: + +- The command is the one reached from the root by taking, in order, every word + that names a subcommand of the command reached so far and skipping every + other word, up to `--`. Its path must occupy the first words of the command + line; otherwise the line is refused with the corrected command, as now. This + reaches at least as deep as `Find`, which descends only through such words, + so neither the walk that repeats pflag's rules nor the cross-check with + `Find` is needed. +- The only ambiguity left is a value placed before the path that names a + subcommand, as in `wit work-items -p get get 1`; such a line is invalid in + any case and is refused. +- Unknown flags before the path are still reported as unknown, looked up on + the command found; `--version` before a path keeps its own message; the + quoting of the corrected command is unchanged. +- The shape of the tree becomes a rule of the brief and is enforced by the + command tree test: a command with subcommands accepts no argument and + defines no flag of its own. Persistent flags stay allowed, since they are + used after the full path. A group that runs on its own, as `git stash` or + `git remote` do, is excluded. + +--- + +### TASK-01 `simple-path` +**Description:** In `internal/cli/flagorder.go`, replace `walkPath`, +`takesValue`, `depth`, and the cross-check with `Find` by the rule above. In +`internal/cli/tree_test.go`, require of every command with subcommands that it +rejects an argument and defines no local non-persistent flag. The test found +that `auth` was built as a plain cobra command rather than with +`newGroupCmd`, so `tfscli auth bogus` printed its help with exit 0; build it +with `newGroupCmd`. +**Definition of done:** Every case in `flagorder_test.go` passes unchanged; +the tree test fails when a group is given a local flag or accepts an +argument, and passes on the current tree. `make check` passes. +**Status:** Done + +### TASK-02 `docs` +**Description:** Add the rule on groups to "The command line can be governed +by permission rules" in `docs/project-brief.md`, to the matching items in +`CLAUDE.md` and `CONTRIBUTING.md`; describe the simpler check and its +dependence on the shape of the tree in `docs/architecture.md`. Record in +`CHANGELOG.md` that an unknown command under `tfscli auth` is now an error. +**Definition of done:** The documents state that a command with subcommands +only groups them, and the architecture no longer describes the walk or the +cross-check with `Find`. `make check` passes. +**Status:** Pending diff --git a/internal/cli/auth.go b/internal/cli/auth.go index de9f183..69df58e 100644 --- a/internal/cli/auth.go +++ b/internal/cli/auth.go @@ -64,10 +64,7 @@ func stdinFd() int { } func newAuthCmd(g *globals) *cobra.Command { - cmd := &cobra.Command{ - Use: "auth", - Short: "Credential commands", - } + cmd := newGroupCmd("auth", "Credential commands") cmd.AddCommand(newAuthLoginCmd(g)) return cmd } diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index fa656e6..808a6c5 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -581,6 +581,11 @@ func TestUnknownCommandBelowTheRootIsAnError(t *testing.T) { args: []string{"wit", "frobnicate"}, want: "Error [config]: unknown command \"frobnicate\" for \"tfscli wit\" (run \"tfscli wit --help\" for the available commands)\n", }, + { + name: "local group", + args: []string{"auth", "frobnicate"}, + want: "Error [config]: unknown command \"frobnicate\" for \"tfscli auth\" (run \"tfscli auth --help\" for the available commands)\n", + }, } for _, tt := range tests { diff --git a/internal/cli/flagorder.go b/internal/cli/flagorder.go index 19e903b..0c41b68 100644 --- a/internal/cli/flagorder.go +++ b/internal/cli/flagorder.go @@ -16,35 +16,38 @@ import ( // permission rules match the command text by its prefix, so the command path // has to open the command line; cobra itself accepts flags at any position. // -// The command is located twice, and the deeper result is taken. Cobra's Find -// decides whether a flag takes a value only from the flags of the command it -// stands on, so a flag unknown there — a flag of an action placed at the level -// of its group — swallows the next word and Find stops at the group. The walk -// in walkPath looks flags up in the whole subtree instead, but it can stop -// where Find goes on: at a word Find skips, such as "" or "-", or at a word -// Find takes as the value of a flag it does not know, such as X in -// "--help X". Whichever command is deeper is the one cobra may run, and it is -// what the check holds to. +// The command is the one reached from the root by taking, in order, every word +// that names a subcommand of the command reached so far, up to "--". No +// knowledge of flag values is needed: a command with subcommands takes no +// arguments and defines no flags of its own (TestEveryGroupOnlyGroups), so in +// a valid command line no word naming a subcommand can follow the path. This +// reaches at least as deep as cobra's Find, which descends only through such +// words, so cobra never runs a command whose path is not at the start. func checkFlagOrder(root *cobra.Command, args []string) error { - target := walkPath(root, args) - if found, _, err := root.Find(args); err == nil && depth(found) > depth(target) { - target = found + cmd := root + var path []int + for i, arg := range args { + if arg == "--" { + break + } + if next := subcommand(cmd, arg); next != nil { + cmd = next + path = append(path, i) + } } - path := strings.Fields(target.CommandPath())[1:] - if len(path) == 0 || slices.Equal(args[:min(len(path), len(args))], path) { + if len(path) == 0 || path[len(path)-1] == len(path)-1 { return nil } // The corrected command is the path followed by every other word in its - // original order; before is the part of it that preceded the path. + // original order; before holds the words that preceded the last path word. + last := path[len(path)-1] var before, rest []string - next := 0 - for _, arg := range args { - if next < len(path) && arg == path[next] { - next++ + for i, arg := range args { + if slices.Contains(path, i) { continue } - if next < len(path) { + if i < last { before = append(before, arg) } rest = append(rest, arg) @@ -53,52 +56,28 @@ func checkFlagOrder(root *cobra.Command, args []string) error { if slices.Contains(before, "--version") { return &tfserr.Error{ Category: tfserr.Config, - Message: fmt.Sprintf("--version belongs to tfscli itself, not to %q (run it as: tfscli --version)", target.CommandPath()), + Message: fmt.Sprintf("--version belongs to tfscli itself, not to %q (run it as: tfscli --version)", cmd.CommandPath()), } } - if err := unknownFlag(target, before); err != nil { + if err := unknownFlag(cmd, before); err != nil { return err } + words := append(strings.Fields(cmd.CommandPath()), rest...) return &tfserr.Error{ Category: tfserr.Config, Message: fmt.Sprintf("the command %q must come first, before its flags and arguments (run it as: %s)", - target.CommandPath(), shellJoin(append(append([]string{root.Name()}, path...), rest...))), + cmd.CommandPath(), shellJoin(words)), } } -// walkPath follows the command path through the arguments from the root and -// returns the deepest command it reaches. A word that follows a flag taking a -// value is that value, whatever it starts with or names, as pflag reads it. -func walkPath(root *cobra.Command, args []string) *cobra.Command { - cmd := root - pendingValue := false - for _, arg := range args { - switch { - case arg == "--": - return cmd - case pendingValue: - pendingValue = false - case strings.HasPrefix(arg, "-") && arg != "-": - name, short, inline := splitFlag(arg) - pendingValue = !inline && takesValue(cmd, name, short) - default: - next := subcommand(cmd, arg) - if next == nil { - return cmd - } - cmd = next +// subcommand returns the child of cmd named name, or nil. +func subcommand(cmd *cobra.Command, name string) *cobra.Command { + for _, c := range cmd.Commands() { + if c.Name() == name { + return c } } - return cmd -} - -// depth is the number of words in the command path of cmd below the root. -func depth(cmd *cobra.Command) int { - n := 0 - for ; cmd.HasParent(); cmd = cmd.Parent() { - n++ - } - return n + return nil } // unknownFlag reports the first flag among words, read as pflag reads them, @@ -128,16 +107,6 @@ func unknownFlag(cmd *cobra.Command, words []string) error { return nil } -// subcommand returns the child of cmd named name, or nil. -func subcommand(cmd *cobra.Command, name string) *cobra.Command { - for _, c := range cmd.Commands() { - if c.Name() == name { - return c - } - } - return nil -} - // splitFlag takes a flag token apart: "--name", "--name=value", "-n", or // "-nvalue". inline reports whether the token carries its value. func splitFlag(arg string) (name string, short, inline bool) { @@ -152,28 +121,6 @@ func splitFlag(arg string) (name string, short, inline bool) { return body, true, false } -// takesValue reports whether the flag named consumes the next argument as its -// value: a flag without a value for its bare form, such as --project, does, -// while a boolean such as --verbose does not. The flag is looked up on cmd, -// among the flags it inherits, and on every command below it, since a flag of -// an action may be placed while the walk still stands on its group. --help, -// -h, and --version, which cobra registers only when it executes a command, -// take no value, and neither does a flag found nowhere. -func takesValue(cmd *cobra.Command, name string, short bool) bool { - if name == "help" || name == "version" || (short && name == "h") { - return false - } - queue := []*cobra.Command{cmd} - for len(queue) > 0 { - c := queue[0] - queue = append(queue[1:], c.Commands()...) - if found, value := lookupFlag(c, name, short); found { - return value - } - } - return false -} - // lookupFlag finds a flag of cmd, its own or inherited, by name or by // shorthand, and reports whether it takes a value. func lookupFlag(cmd *cobra.Command, name string, short bool) (found, takesValue bool) { diff --git a/internal/cli/tree_test.go b/internal/cli/tree_test.go index 8dc3afa..bd607c4 100644 --- a/internal/cli/tree_test.go +++ b/internal/cli/tree_test.go @@ -26,3 +26,29 @@ func TestEveryCommandHasOneForm(t *testing.T) { } walk(newRoot(testBuild, noTerminal{}, io.Discard, io.Discard)) } + +// checkFlagOrder takes every word naming a subcommand as a step of the path, +// which holds only while a command with subcommands just groups them: a +// group's own argument or flag value could otherwise name a subcommand and be +// taken for one. Persistent flags are allowed, since they are used after the +// full path. +func TestEveryGroupOnlyGroups(t *testing.T) { + var walk func(cmd *cobra.Command) + walk = func(cmd *cobra.Command) { + if !cmd.HasSubCommands() { + return + } + // Cobra itself rejects an argument to the root that names no + // subcommand, when the root leaves Args unset. + if (cmd.HasParent() || cmd.Args != nil) && cmd.ValidateArgs([]string{"x"}) == nil { + t.Errorf("%q accepts an argument; a command with subcommands takes none", cmd.CommandPath()) + } + if own := cmd.LocalNonPersistentFlags(); own.HasFlags() { + t.Errorf("%q defines flags of its own; a command with subcommands has none:\n%s", cmd.CommandPath(), own.FlagUsages()) + } + for _, child := range cmd.Commands() { + walk(child) + } + } + walk(newRoot(testBuild, noTerminal{}, io.Discard, io.Discard)) +} From 6c6b5383b347e8986373bc9ec2a34320e2646927 Mon Sep 17 00:00:00 2001 From: Dmitry Pleshakov Date: Fri, 9 Oct 2026 08:48:14 +0300 Subject: [PATCH 12/25] Record that a command with subcommands only groups them (TASK-02) The flag order check now relies on the shape of the command tree, so the shape becomes a rule of the brief, repeated in CLAUDE.md and CONTRIBUTING: a command with subcommands takes no arguments, defines no flags of its own, and only prints its help. A group that also acts on its own, as git stash does, is excluded, since a rule written for the group would cover every command below it. The architecture describes the check without the walk over flag values and records why that walk was rejected. The changelog records the fix to unknown commands under auth. Closes docs/archive/2026-10-09-tasks-simple-path-rule.md Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01XA4Hx4kZtgobXtNCEpuiTC --- CHANGELOG.md | 1 + CLAUDE.md | 2 +- CONTRIBUTING.md | 3 ++- docs/architecture.md | 2 +- docs/{ => archive}/2026-10-09-tasks-simple-path-rule.md | 4 ++-- docs/project-brief.md | 1 + 6 files changed, 8 insertions(+), 5 deletions(-) rename docs/{ => archive}/2026-10-09-tasks-simple-path-rule.md (98%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 51eb73e..8c371b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - A flag of an action placed before the action, as in `tfscli wit wiql --time-precision query-by-wiql`, no longer makes tfscli print the help of the group and exit successfully without running the query. +- An unknown command under `tfscli auth`, such as `tfscli auth logn`, is now reported as an unknown command, where tfscli used to print the help of `auth` and exit successfully. ## [0.1.1] - 2026-10-06 diff --git a/CLAUDE.md b/CLAUDE.md index 3903954..7291ed7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -23,7 +23,7 @@ These come from `project-brief.md` and override casual feature requests. If a ch - **PAT auth only.** No SSPI or NTLM. A PAT is issued for one collection, so the server URL, the collection, and the PAT are stored together and always come from one source: `$XDG_DATA_HOME/tfscli/auth.json` (default `~/.local/share/tfscli/auth.json`, mode `0600`, written only by the interactive `tfscli auth login`) or `TFSCLI_AUTH` holding the same JSON, which takes precedence. No flag, separate env var, or config key may supply the URL, the collection, or the token. No OS keychain unless an organisation requires one. - **Read-only in v1.** Write operations are explicitly out of scope. They additionally depend on JSON output landing first, because markdown is lossy. - **Write operations are gated on the command line.** When they are added, every operation that changes server state requires `--allow-changes`, and an irreversible one (its effect cannot be undone by another API operation) requires `--allow-irreversible` as well. Writing is a property of the command, never of a flag of a reading command. The gate flags have no short form, environment variable, or config key, and are checked before any request. See "Write operations" in Non-Goals of `project-brief.md`. -- **The command line stays matchable by agent permission rules.** Flags follow the last word of the command path; each command has one form (no aliases, no abbreviations, no prefix matching); local command names never coincide with REST API areas; no generic pass-through command. See "The command line can be governed by permission rules" in `project-brief.md`. +- **The command line stays matchable by agent permission rules.** Flags follow the last word of the command path; each command has one form (no aliases, no abbreviations, no prefix matching); a command with subcommands only groups them (no arguments, no flags of its own, nothing but its help); local command names never coincide with REST API areas; no generic pass-through command. See "The command line can be governed by permission rules" in `project-brief.md`. - **Markdown is the default output.** HTML fields (Description, ReproSteps, etc.) are converted via `github.com/JohannesKaufmann/html-to-markdown/v2`. JSON output is a planned `--json` flag, not present in v1. - **Mirror TFS REST API structure.** Command hierarchy, parameter names, and behaviour follow the API. Any deviation needs explicit usability justification — don't invent new abstractions (no custom query syntax, no synthetic batch endpoints, no caching). - **Stateless.** No daemon, no background process, no on-disk cache. Every call hits the server. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 3c94ea0..603a808 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -83,7 +83,8 @@ if a change would break one, raise it before implementing. - The command hierarchy and parameter names mirror the TFS REST API; a deviation needs an explicit usability justification, and no query syntax or batch endpoint is invented. - The command line stays matchable by agent permission rules: flags follow the command - path, each command has one form (no aliases, abbreviations, or prefix matching), local + path, each command has one form (no aliases, abbreviations, or prefix matching), a + command with subcommands only groups them (no arguments, no flags of its own), local command names never coincide with REST API areas, and there is no generic pass-through command. - Stateless: no daemon, no background process, no on-disk state beyond the config file and the credential written by `auth login`. diff --git a/docs/architecture.md b/docs/architecture.md index e461807..0e6c4bd 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -195,7 +195,7 @@ This section describes the conceptual module structure, their responsibilities, ### Modules and responsibilities -- **cli** — cobra commands, nested as area, resource, and action after the REST API reference (`wit` → `work-items` → `get`); persistent flags (`--verbose`, config overrides); orchestrates the chain config → apiclient → domain → printer → exit. Owns the markdown printer functions (one per resource and shape, such as a single work item and several) plus small per-kind scalar formatters for identity and datetime values used by those printers. No `Renderer` interface in v1 — printers are plain functions. When `--json` is added later, the interface and a second renderer can be introduced here without touching the domain layer. Enforces the command line shape that "The command line can be governed by permission rules" in `project-brief.md` requires. Cobra accepts flags at any position, so `run` checks the arguments before executing the tree: the path of the command to be run must occupy exactly the first words of the command line, and otherwise the command fails with a `config` error that shows the corrected command — the path followed by every other word in its original order, quoted for the shell in single quotes, which bash and PowerShell read alike. The command is located twice, and the deeper result is taken. Cobra's `Find` decides whether a flag takes a value only from the flags of the command it stands on, so a flag of an action placed at the level of its group swallows the next word and `Find` stops at the group. The walk in `walkPath` looks a flag up on the current command and on every command below it, treats `--help`, `-h`, and `--version` as taking no value, and reads the word after a flag with a value as that value whatever it starts with or names, as pflag does; but it stops where `Find` goes on, at a word `Find` skips (`""`, `-`) or takes as the value of a flag it does not know (`--help X`). Each catches the other stopping early. `newRoot` registers cobra's `help` and `completion` commands up front so that both treat them like any other command. A flag before the path that the command does not define is reported as unknown, in pflag's wording, rather than moved into a corrected command that would fail the same way; `--version`, defined on the root alone, is refused before a path with a message that names `tfscli --version`. The check is explicit rather than a root `PersistentPreRunE`, because cobra runs only the nearest such hook and a child command defining its own would silently drop it. A test walks the whole command tree and fails on any command alias or on cobra's prefix matching, so that each command keeps exactly one form. +- **cli** — cobra commands, nested as area, resource, and action after the REST API reference (`wit` → `work-items` → `get`); persistent flags (`--verbose`, config overrides); orchestrates the chain config → apiclient → domain → printer → exit. Owns the markdown printer functions (one per resource and shape, such as a single work item and several) plus small per-kind scalar formatters for identity and datetime values used by those printers. No `Renderer` interface in v1 — printers are plain functions. When `--json` is added later, the interface and a second renderer can be introduced here without touching the domain layer. Enforces the command line shape that "The command line can be governed by permission rules" in `project-brief.md` requires. Cobra accepts flags at any position, so `run` checks the arguments before executing the tree: the path of the command must occupy exactly the first words of the command line, and otherwise the command fails with a `config` error that shows the corrected command — the path followed by every other word in its original order, quoted for the shell in single quotes, which bash and PowerShell read alike. The command is the one reached from the root by taking, in order, every word that names a subcommand of the command reached so far, up to `--`; flag values are not read. This depends on the shape of the tree: a command with subcommands takes no arguments and defines no flags of its own, so in a valid command line no word naming a subcommand can follow the path, and the rule reaches at least as deep as cobra's `Find`, which descends only through such words. A rule that read flag values to find the path was rejected: it is a second parser of the command line, and each of its divergences from pflag let a command run with its path not at the start. `newRoot` registers cobra's `help` and `completion` commands up front so that they are found like any other command. A flag before the path that the command does not define is reported as unknown, in pflag's wording, rather than moved into a corrected command that would fail the same way; `--version`, defined on the root alone, is refused before a path with a message that names `tfscli --version`. The check is explicit rather than a root `PersistentPreRunE`, because cobra runs only the nearest such hook and a child command defining its own would silently drop it. A test walks the whole command tree and fails on any command alias, on cobra's prefix matching, and on a command with subcommands that accepts an argument or defines a flag of its own, so that each command keeps exactly one form and the check keeps the shape it relies on. Every group, local ones included, is built with `newGroupCmd`. - **config** — resolves the credential (`URL`, `Collection`, and `PAT`; a PAT is issued for one collection) from `TFSCLI_AUTH` or `$XDG_DATA_HOME/tfscli/auth.json`, never from a mix of sources; loads the optional `$XDG_CONFIG_HOME/tfscli/config.json`, applies environment overrides (`TFSCLI_*`), applies cobra flag overrides; validates that all required fields are set for the command being run. Writes `auth.json` for `auth login` (mode `0600`, directory `0700`). Holds `URL`, `Collection`, `PAT`, `Project`, `APIVersion`, `InsecureSkipVerify`, `CABundle`. Never logs PAT. - **apiclient** — wraps `net/http`. Builds URLs from `Config.URL + Collection + path` and adds `api-version`. A configured `APIVersion` is sent as it is. Otherwise the version is negotiated per request, as the official SDKs do: `OPTIONS {URL}/{Collection}/_apis` lists every resource location with its versions, the caller names its location by id, and the request carries the location's released version, or `{maxVersion}-preview.{resourceVersion}` when it has none — the version the server chooses for a request without one. The response is not cached. A 401 or a transport failure on `OPTIONS` fails the request; any other failure sends it without a version and notes why through `Logger.Info`, and a refusal of that request for the missing version gets the settings to use in its message. A call without a location, the login check, neither negotiates nor sends a version. Sets `Authorization: Basic base64(":")` on every request. Configures TLS using `CABundle` (appended to system root pool) and `InsecureSkipVerify`. Hooks the logger via a `RoundTripper` — that transport is the single call site of `LogRequest`, so every outgoing request is logged exactly once and no other module logs HTTP traffic. Classifies HTTP outcomes into the stable error categories defined in the brief (`auth`, `not_found`, `forbidden`, `server`, `config`, `network`). - **workitem** — domain logic for Work Items: Get Work Item, Work Items - List, and Get Work Items Batch. Owns the `WorkItem`/`Field` types, the `BatchRequest`/`Batch` types for reading several work items, and the allowlist of fields known to contain HTML (`System.Description`, `Microsoft.VSTS.TCM.ReproSteps`, `Microsoft.VSTS.TCM.SystemInfo`, `Microsoft.VSTS.Common.AcceptanceCriteria`, …). After unmarshalling, tags each field with its `FieldKind`. For several work items, keeps the server's order and reports the requested ids the server did not return. Returns raw values — does not perform markdown conversion. diff --git a/docs/2026-10-09-tasks-simple-path-rule.md b/docs/archive/2026-10-09-tasks-simple-path-rule.md similarity index 98% rename from docs/2026-10-09-tasks-simple-path-rule.md rename to docs/archive/2026-10-09-tasks-simple-path-rule.md index 50c344e..78a2a43 100644 --- a/docs/2026-10-09-tasks-simple-path-rule.md +++ b/docs/archive/2026-10-09-tasks-simple-path-rule.md @@ -1,6 +1,6 @@ # 2026-10-09-tasks-simple-path-rule.md -**Status:** Active +**Status:** Archived ## Context @@ -62,4 +62,4 @@ dependence on the shape of the tree in `docs/architecture.md`. Record in **Definition of done:** The documents state that a command with subcommands only groups them, and the architecture no longer describes the walk or the cross-check with `Find`. `make check` passes. -**Status:** Pending +**Status:** Done diff --git a/docs/project-brief.md b/docs/project-brief.md index 1e8c050..585955f 100644 --- a/docs/project-brief.md +++ b/docs/project-brief.md @@ -63,6 +63,7 @@ An AI agent can call `tfscli`, read work item content, and use it meaningfully i - **The command line can be governed by permission rules.** AI agent hosts, such as Claude Code and opencode, decide whether an agent may run a command by matching its text against allow, ask, and deny rules. tfscli keeps that text predictable: - The command path opens the command line, and flags follow its last word: `tfscli wit work-items get --verbose 12345`, not `tfscli --verbose wit work-items get 12345`. A flag placed earlier is refused with a `config` error that shows the corrected command. The rule has no exceptions: `--help` follows the path like any other flag, and the built-in `help` and `completion` commands are commands like any other. Only an invocation without a command path, such as `tfscli --help` or `tfscli --version`, has no path for a flag to precede; `--version` belongs to the root alone. - Each command has exactly one form: no command aliases, no abbreviated command or flag names, no prefix matching of command names. + - A command that has subcommands — an area, a resource, or a local group such as `auth` — only groups them: it takes no arguments, defines no flags of its own, and does nothing but print its help. Only a command without subcommands runs. A group that also acts on its own, as `git stash` and `git remote` do, is excluded: a rule written for the group would also cover every command below it, and the command path could no longer be told from the group's own arguments. - The names of local commands (`auth`, `licenses`, `help`, `completion`) never coincide with an area of the REST API reference. - Whether a command changes server state is a property of the command, never of its flags: a reading command does not gain a side effect through a flag. There is no generic pass-through command that sends an arbitrary request; should one be added, it falls under the gate flags described in "Non-Goals". - **Predictable error output.** Errors go to stderr with a machine-readable category and human-readable message. Exit code is non-zero. Format: `Error [category]: message (HTTP status)`. Error categories are a stable contract: existing categories are never removed or renamed; new categories may be added in future versions. From 74608430088d6ffd457d2e49d526454c9af3ce89 Mon Sep 17 00:00:00 2001 From: Dmitry Pleshakov Date: Fri, 9 Oct 2026 08:57:02 +0300 Subject: [PATCH 13/25] Let shell completion requests through the flag order check (TASK-01) Cobra adds its hidden __complete command only when it executes the tree, so the check took "tfscli __complete wit ''" for a misplaced path and broke shell completion. Such a request runs no command. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01XA4Hx4kZtgobXtNCEpuiTC --- docs/2026-10-09-tasks-flag-order-oracle.md | 61 ++++++++++++++++++++++ docs/architecture.md | 2 +- internal/cli/flagorder.go | 7 +++ internal/cli/flagorder_test.go | 26 +++++++++ 4 files changed, 95 insertions(+), 1 deletion(-) create mode 100644 docs/2026-10-09-tasks-flag-order-oracle.md diff --git a/docs/2026-10-09-tasks-flag-order-oracle.md b/docs/2026-10-09-tasks-flag-order-oracle.md new file mode 100644 index 0000000..28db5eb --- /dev/null +++ b/docs/2026-10-09-tasks-flag-order-oracle.md @@ -0,0 +1,61 @@ +# 2026-10-09-tasks-flag-order-oracle.md + +**Status:** Active + +## Context + +The tests of the flag order check compare it with expectations written by +hand. The defects found in review were cases where cobra treated a command +line differently from what those expectations assumed, so the tests could not +catch them. This work adds a test that takes cobra itself as the oracle and +runs it over command lines generated rather than chosen. + +The behaviour is already decided; the test restates the rule in a form that +can be checked on any command line: + +- If `checkFlagOrder` accepts a command line, the command cobra selects for it + (`ExecuteC`, with every command's action replaced by a stub) has its path in + the first words of the line. +- If `checkFlagOrder` refuses a command line because the path does not come + first, the corrected command line it proposes is accepted by the check, and + cobra selects for it the command named in the message. + +The converse of the first property does not hold by design: a line such as +`tfscli --verbose wit work-items get 1` is refused although cobra would run +it. + +Command lines are built from an alphabet derived from the command tree (every +command name and every flag in its long, short, and inline forms) and a fixed +set of special words (`--`, `-`, the empty word, `--help`, `-h`, +`--version`, an unknown flag, a plain word, a number). All lines up to +length 3 are checked exhaustively, and a fixed-seed random sample covers +lengths 4 to 7, so the test stays deterministic. + +While preparing this, shell completion was found broken on this branch: +cobra adds its hidden `__complete` command only when it executes the tree, so +`tfscli __complete wit ""`, which the completion scripts run, is refused as a +misplaced path. The defect was never released, so it needs no changelog entry. + +--- + +### TASK-01 `completion-request` +**Description:** In `checkFlagOrder`, accept a command line whose first word +is `cobra.ShellCompRequestCmd` or `cobra.ShellCompNoDescRequestCmd`: such a +request only lists completions and runs no command. Add a test case and a +sentence on it to the cli module paragraph of `docs/architecture.md`. +**Definition of done:** `tfscli __complete wit ""` lists the subcommands of +`wit` with exit 0, covered by a test. `make check` passes. +**Status:** Done + +### TASK-02 `oracle-test` +**Description:** Add the test described in Context to +`internal/cli/flagorder_test.go`, building a fresh command tree for every +command line. Factor the construction of the corrected command line out of +`checkFlagOrder` so that the test can run it as words rather than parse the +message. Mention the test in the cli module paragraph of +`docs/architecture.md`. +**Definition of done:** The test passes on the current code and fails when +the check is weakened, for example when it stops at the first word that is +not a subcommand. `make check` passes, and the test adds no more than a few +seconds to it. +**Status:** Pending diff --git a/docs/architecture.md b/docs/architecture.md index 0e6c4bd..21f050f 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -195,7 +195,7 @@ This section describes the conceptual module structure, their responsibilities, ### Modules and responsibilities -- **cli** — cobra commands, nested as area, resource, and action after the REST API reference (`wit` → `work-items` → `get`); persistent flags (`--verbose`, config overrides); orchestrates the chain config → apiclient → domain → printer → exit. Owns the markdown printer functions (one per resource and shape, such as a single work item and several) plus small per-kind scalar formatters for identity and datetime values used by those printers. No `Renderer` interface in v1 — printers are plain functions. When `--json` is added later, the interface and a second renderer can be introduced here without touching the domain layer. Enforces the command line shape that "The command line can be governed by permission rules" in `project-brief.md` requires. Cobra accepts flags at any position, so `run` checks the arguments before executing the tree: the path of the command must occupy exactly the first words of the command line, and otherwise the command fails with a `config` error that shows the corrected command — the path followed by every other word in its original order, quoted for the shell in single quotes, which bash and PowerShell read alike. The command is the one reached from the root by taking, in order, every word that names a subcommand of the command reached so far, up to `--`; flag values are not read. This depends on the shape of the tree: a command with subcommands takes no arguments and defines no flags of its own, so in a valid command line no word naming a subcommand can follow the path, and the rule reaches at least as deep as cobra's `Find`, which descends only through such words. A rule that read flag values to find the path was rejected: it is a second parser of the command line, and each of its divergences from pflag let a command run with its path not at the start. `newRoot` registers cobra's `help` and `completion` commands up front so that they are found like any other command. A flag before the path that the command does not define is reported as unknown, in pflag's wording, rather than moved into a corrected command that would fail the same way; `--version`, defined on the root alone, is refused before a path with a message that names `tfscli --version`. The check is explicit rather than a root `PersistentPreRunE`, because cobra runs only the nearest such hook and a child command defining its own would silently drop it. A test walks the whole command tree and fails on any command alias, on cobra's prefix matching, and on a command with subcommands that accepts an argument or defines a flag of its own, so that each command keeps exactly one form and the check keeps the shape it relies on. Every group, local ones included, is built with `newGroupCmd`. +- **cli** — cobra commands, nested as area, resource, and action after the REST API reference (`wit` → `work-items` → `get`); persistent flags (`--verbose`, config overrides); orchestrates the chain config → apiclient → domain → printer → exit. Owns the markdown printer functions (one per resource and shape, such as a single work item and several) plus small per-kind scalar formatters for identity and datetime values used by those printers. No `Renderer` interface in v1 — printers are plain functions. When `--json` is added later, the interface and a second renderer can be introduced here without touching the domain layer. Enforces the command line shape that "The command line can be governed by permission rules" in `project-brief.md` requires. Cobra accepts flags at any position, so `run` checks the arguments before executing the tree: the path of the command must occupy exactly the first words of the command line, and otherwise the command fails with a `config` error that shows the corrected command — the path followed by every other word in its original order, quoted for the shell in single quotes, which bash and PowerShell read alike. The command is the one reached from the root by taking, in order, every word that names a subcommand of the command reached so far, up to `--`; flag values are not read. This depends on the shape of the tree: a command with subcommands takes no arguments and defines no flags of its own, so in a valid command line no word naming a subcommand can follow the path, and the rule reaches at least as deep as cobra's `Find`, which descends only through such words. A rule that read flag values to find the path was rejected: it is a second parser of the command line, and each of its divergences from pflag let a command run with its path not at the start. `newRoot` registers cobra's `help` and `completion` commands up front so that they are found like any other command. Cobra's hidden `__complete` command, which the shell completion scripts run, is added only when cobra executes the tree, so a command line that starts with it is let through: it lists completions for the words after it and runs no command. A flag before the path that the command does not define is reported as unknown, in pflag's wording, rather than moved into a corrected command that would fail the same way; `--version`, defined on the root alone, is refused before a path with a message that names `tfscli --version`. The check is explicit rather than a root `PersistentPreRunE`, because cobra runs only the nearest such hook and a child command defining its own would silently drop it. A test walks the whole command tree and fails on any command alias, on cobra's prefix matching, and on a command with subcommands that accepts an argument or defines a flag of its own, so that each command keeps exactly one form and the check keeps the shape it relies on. Every group, local ones included, is built with `newGroupCmd`. - **config** — resolves the credential (`URL`, `Collection`, and `PAT`; a PAT is issued for one collection) from `TFSCLI_AUTH` or `$XDG_DATA_HOME/tfscli/auth.json`, never from a mix of sources; loads the optional `$XDG_CONFIG_HOME/tfscli/config.json`, applies environment overrides (`TFSCLI_*`), applies cobra flag overrides; validates that all required fields are set for the command being run. Writes `auth.json` for `auth login` (mode `0600`, directory `0700`). Holds `URL`, `Collection`, `PAT`, `Project`, `APIVersion`, `InsecureSkipVerify`, `CABundle`. Never logs PAT. - **apiclient** — wraps `net/http`. Builds URLs from `Config.URL + Collection + path` and adds `api-version`. A configured `APIVersion` is sent as it is. Otherwise the version is negotiated per request, as the official SDKs do: `OPTIONS {URL}/{Collection}/_apis` lists every resource location with its versions, the caller names its location by id, and the request carries the location's released version, or `{maxVersion}-preview.{resourceVersion}` when it has none — the version the server chooses for a request without one. The response is not cached. A 401 or a transport failure on `OPTIONS` fails the request; any other failure sends it without a version and notes why through `Logger.Info`, and a refusal of that request for the missing version gets the settings to use in its message. A call without a location, the login check, neither negotiates nor sends a version. Sets `Authorization: Basic base64(":")` on every request. Configures TLS using `CABundle` (appended to system root pool) and `InsecureSkipVerify`. Hooks the logger via a `RoundTripper` — that transport is the single call site of `LogRequest`, so every outgoing request is logged exactly once and no other module logs HTTP traffic. Classifies HTTP outcomes into the stable error categories defined in the brief (`auth`, `not_found`, `forbidden`, `server`, `config`, `network`). - **workitem** — domain logic for Work Items: Get Work Item, Work Items - List, and Get Work Items Batch. Owns the `WorkItem`/`Field` types, the `BatchRequest`/`Batch` types for reading several work items, and the allowlist of fields known to contain HTML (`System.Description`, `Microsoft.VSTS.TCM.ReproSteps`, `Microsoft.VSTS.TCM.SystemInfo`, `Microsoft.VSTS.Common.AcceptanceCriteria`, …). After unmarshalling, tags each field with its `FieldKind`. For several work items, keeps the server's order and reports the requested ids the server did not return. Returns raw values — does not perform markdown conversion. diff --git a/internal/cli/flagorder.go b/internal/cli/flagorder.go index 0c41b68..8229048 100644 --- a/internal/cli/flagorder.go +++ b/internal/cli/flagorder.go @@ -23,7 +23,14 @@ import ( // a valid command line no word naming a subcommand can follow the path. This // reaches at least as deep as cobra's Find, which descends only through such // words, so cobra never runs a command whose path is not at the start. +// +// A shell completion request, which cobra serves with a hidden command it adds +// only when it executes the tree, is let through: it lists completions for the +// words after it and runs no command. func checkFlagOrder(root *cobra.Command, args []string) error { + if len(args) > 0 && (args[0] == cobra.ShellCompRequestCmd || args[0] == cobra.ShellCompNoDescRequestCmd) { + return nil + } cmd := root var path []int for i, arg := range args { diff --git a/internal/cli/flagorder_test.go b/internal/cli/flagorder_test.go index f03ab6d..f520c1a 100644 --- a/internal/cli/flagorder_test.go +++ b/internal/cli/flagorder_test.go @@ -188,3 +188,29 @@ func TestHelpAndVersionAfterThePath(t *testing.T) { }) } } + +// The completion scripts ask for completions with cobra's hidden __complete +// command followed by the words typed so far; such a request runs no command, +// so the words after it need not start with a command path. +func TestShellCompletionRequest(t *testing.T) { + tests := []struct { + args []string + want string + }{ + {args: []string{"__complete", "wit", ""}, want: "work-items\t"}, + {args: []string{"__completeNoDesc", "wit", "wiql", ""}, want: "query-by-wiql\n"}, + } + + for _, tt := range tests { + t.Run(strings.Join(tt.args, " "), func(t *testing.T) { + stdout, stderr, code := execute(t, nil, tt.args...) + + if code != 0 { + t.Fatalf("exit code = %d, want 0 (stderr: %s)", code, stderr) + } + if !strings.Contains(stdout, tt.want) { + t.Errorf("stdout does not contain %q:\n%s", tt.want, stdout) + } + }) + } +} From 1bc212761d793cd859060129909485bec995c37e Mon Sep 17 00:00:00 2001 From: Dmitry Pleshakov Date: Fri, 9 Oct 2026 09:15:41 +0300 Subject: [PATCH 14/25] Check the flag order rule against cobra as an oracle (TASK-02) A test runs the check and cobra over every command line of up to three words built from the names and flags of the tree, and over a fixed-seed sample of longer lines: a line the check accepts must make cobra select a command whose path opens the line, and the corrected command proposed for a refused line must itself be accepted and select the command the message names. The test found three divergences, now fixed: - "--" no longer stops the search for the path, since pflag takes it for the value of a flag before it, as in "tfscli --api-version -- completion"; - a flag before "__complete" is refused like any other, the completion request being taken for a subcommand of the root that ends the path; - the path is moved to the front until it stays there, so that the correction of "tfscli - login auth" is "tfscli auth login -" rather than a line refused again. Closes docs/archive/2026-10-09-tasks-flag-order-oracle.md Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01XA4Hx4kZtgobXtNCEpuiTC --- docs/architecture.md | 2 +- .../2026-10-09-tasks-flag-order-oracle.md | 4 +- internal/cli/flagorder.go | 88 ++++++--- internal/cli/flagorder_test.go | 186 ++++++++++++++++++ 4 files changed, 250 insertions(+), 30 deletions(-) rename docs/{ => archive}/2026-10-09-tasks-flag-order-oracle.md (98%) diff --git a/docs/architecture.md b/docs/architecture.md index 21f050f..6f66d77 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -195,7 +195,7 @@ This section describes the conceptual module structure, their responsibilities, ### Modules and responsibilities -- **cli** — cobra commands, nested as area, resource, and action after the REST API reference (`wit` → `work-items` → `get`); persistent flags (`--verbose`, config overrides); orchestrates the chain config → apiclient → domain → printer → exit. Owns the markdown printer functions (one per resource and shape, such as a single work item and several) plus small per-kind scalar formatters for identity and datetime values used by those printers. No `Renderer` interface in v1 — printers are plain functions. When `--json` is added later, the interface and a second renderer can be introduced here without touching the domain layer. Enforces the command line shape that "The command line can be governed by permission rules" in `project-brief.md` requires. Cobra accepts flags at any position, so `run` checks the arguments before executing the tree: the path of the command must occupy exactly the first words of the command line, and otherwise the command fails with a `config` error that shows the corrected command — the path followed by every other word in its original order, quoted for the shell in single quotes, which bash and PowerShell read alike. The command is the one reached from the root by taking, in order, every word that names a subcommand of the command reached so far, up to `--`; flag values are not read. This depends on the shape of the tree: a command with subcommands takes no arguments and defines no flags of its own, so in a valid command line no word naming a subcommand can follow the path, and the rule reaches at least as deep as cobra's `Find`, which descends only through such words. A rule that read flag values to find the path was rejected: it is a second parser of the command line, and each of its divergences from pflag let a command run with its path not at the start. `newRoot` registers cobra's `help` and `completion` commands up front so that they are found like any other command. Cobra's hidden `__complete` command, which the shell completion scripts run, is added only when cobra executes the tree, so a command line that starts with it is let through: it lists completions for the words after it and runs no command. A flag before the path that the command does not define is reported as unknown, in pflag's wording, rather than moved into a corrected command that would fail the same way; `--version`, defined on the root alone, is refused before a path with a message that names `tfscli --version`. The check is explicit rather than a root `PersistentPreRunE`, because cobra runs only the nearest such hook and a child command defining its own would silently drop it. A test walks the whole command tree and fails on any command alias, on cobra's prefix matching, and on a command with subcommands that accepts an argument or defines a flag of its own, so that each command keeps exactly one form and the check keeps the shape it relies on. Every group, local ones included, is built with `newGroupCmd`. +- **cli** — cobra commands, nested as area, resource, and action after the REST API reference (`wit` → `work-items` → `get`); persistent flags (`--verbose`, config overrides); orchestrates the chain config → apiclient → domain → printer → exit. Owns the markdown printer functions (one per resource and shape, such as a single work item and several) plus small per-kind scalar formatters for identity and datetime values used by those printers. No `Renderer` interface in v1 — printers are plain functions. When `--json` is added later, the interface and a second renderer can be introduced here without touching the domain layer. Enforces the command line shape that "The command line can be governed by permission rules" in `project-brief.md` requires. Cobra accepts flags at any position, so `run` checks the arguments before executing the tree: the path of the command must occupy exactly the first words of the command line, and otherwise the command fails with a `config` error that shows the corrected command — the path followed by every other word in its original order, quoted for the shell in single quotes, which bash and PowerShell read alike. The command is the one reached from the root by taking, in order, every word that names a subcommand of the command reached so far; flag values are not read, and `--` does not stop the search, since pflag takes it for the value of a flag placed before it. When moving the path to the front turns a word that preceded one of its steps into a further step, as `login` in `tfscli - login auth`, the move is repeated, so that the corrected command is itself accepted. This depends on the shape of the tree: a command with subcommands takes no arguments and defines no flags of its own, so in a valid command line no word naming a subcommand can follow the path, and the rule reaches at least as deep as cobra's `Find`, which descends only through such words. A rule that read flag values to find the path was rejected: it is a second parser of the command line, and each of its divergences from pflag let a command run with its path not at the start. `newRoot` registers cobra's `help` and `completion` commands up front so that they are found like any other command. Cobra's hidden `__complete` command, which the shell completion scripts run, is added only when cobra executes the tree, so the check takes it for a subcommand of the root that ends the path: the words after it are the command line to complete. A flag before the path that the command does not define is reported as unknown, in pflag's wording, rather than moved into a corrected command that would fail the same way; `--version`, defined on the root alone, is refused before a path with a message that names `tfscli --version`. The check is explicit rather than a root `PersistentPreRunE`, because cobra runs only the nearest such hook and a child command defining its own would silently drop it. A test walks the whole command tree and fails on any command alias, on cobra's prefix matching, and on a command with subcommands that accepts an argument or defines a flag of its own, so that each command keeps exactly one form and the check keeps the shape it relies on. Another test takes cobra as the oracle: over every command line of up to three words built from the names and flags of the tree and the words pflag treats specially, and a fixed-seed sample of longer lines, it requires that a line the check accepts makes cobra select a command whose path opens the line, and that the corrected command proposed for a refused line is accepted and makes cobra select the command the message names. Every group, local ones included, is built with `newGroupCmd`. - **config** — resolves the credential (`URL`, `Collection`, and `PAT`; a PAT is issued for one collection) from `TFSCLI_AUTH` or `$XDG_DATA_HOME/tfscli/auth.json`, never from a mix of sources; loads the optional `$XDG_CONFIG_HOME/tfscli/config.json`, applies environment overrides (`TFSCLI_*`), applies cobra flag overrides; validates that all required fields are set for the command being run. Writes `auth.json` for `auth login` (mode `0600`, directory `0700`). Holds `URL`, `Collection`, `PAT`, `Project`, `APIVersion`, `InsecureSkipVerify`, `CABundle`. Never logs PAT. - **apiclient** — wraps `net/http`. Builds URLs from `Config.URL + Collection + path` and adds `api-version`. A configured `APIVersion` is sent as it is. Otherwise the version is negotiated per request, as the official SDKs do: `OPTIONS {URL}/{Collection}/_apis` lists every resource location with its versions, the caller names its location by id, and the request carries the location's released version, or `{maxVersion}-preview.{resourceVersion}` when it has none — the version the server chooses for a request without one. The response is not cached. A 401 or a transport failure on `OPTIONS` fails the request; any other failure sends it without a version and notes why through `Logger.Info`, and a refusal of that request for the missing version gets the settings to use in its message. A call without a location, the login check, neither negotiates nor sends a version. Sets `Authorization: Basic base64(":")` on every request. Configures TLS using `CABundle` (appended to system root pool) and `InsecureSkipVerify`. Hooks the logger via a `RoundTripper` — that transport is the single call site of `LogRequest`, so every outgoing request is logged exactly once and no other module logs HTTP traffic. Classifies HTTP outcomes into the stable error categories defined in the brief (`auth`, `not_found`, `forbidden`, `server`, `config`, `network`). - **workitem** — domain logic for Work Items: Get Work Item, Work Items - List, and Get Work Items Batch. Owns the `WorkItem`/`Field` types, the `BatchRequest`/`Batch` types for reading several work items, and the allowlist of fields known to contain HTML (`System.Description`, `Microsoft.VSTS.TCM.ReproSteps`, `Microsoft.VSTS.TCM.SystemInfo`, `Microsoft.VSTS.Common.AcceptanceCriteria`, …). After unmarshalling, tags each field with its `FieldKind`. For several work items, keeps the server's order and reports the requested ids the server did not return. Returns raw values — does not perform markdown conversion. diff --git a/docs/2026-10-09-tasks-flag-order-oracle.md b/docs/archive/2026-10-09-tasks-flag-order-oracle.md similarity index 98% rename from docs/2026-10-09-tasks-flag-order-oracle.md rename to docs/archive/2026-10-09-tasks-flag-order-oracle.md index 28db5eb..1b68ed9 100644 --- a/docs/2026-10-09-tasks-flag-order-oracle.md +++ b/docs/archive/2026-10-09-tasks-flag-order-oracle.md @@ -1,6 +1,6 @@ # 2026-10-09-tasks-flag-order-oracle.md -**Status:** Active +**Status:** Archived ## Context @@ -58,4 +58,4 @@ message. Mention the test in the cli module paragraph of the check is weakened, for example when it stops at the first word that is not a subcommand. `make check` passes, and the test adds no more than a few seconds to it. -**Status:** Pending +**Status:** Done diff --git a/internal/cli/flagorder.go b/internal/cli/flagorder.go index 8229048..bf2e601 100644 --- a/internal/cli/flagorder.go +++ b/internal/cli/flagorder.go @@ -17,41 +17,90 @@ import ( // has to open the command line; cobra itself accepts flags at any position. // // The command is the one reached from the root by taking, in order, every word -// that names a subcommand of the command reached so far, up to "--". No +// that names a subcommand of the command reached so far, "--" not excepted, +// since pflag takes it for the value of a flag before it. No // knowledge of flag values is needed: a command with subcommands takes no // arguments and defines no flags of its own (TestEveryGroupOnlyGroups), so in // a valid command line no word naming a subcommand can follow the path. This // reaches at least as deep as cobra's Find, which descends only through such // words, so cobra never runs a command whose path is not at the start. // -// A shell completion request, which cobra serves with a hidden command it adds -// only when it executes the tree, is let through: it lists completions for the -// words after it and runs no command. +// Cobra's hidden command for shell completion requests, which it adds to the +// root only when it executes the tree, is taken as a subcommand of the root +// that ends the path: the words after it are the command line to complete. func checkFlagOrder(root *cobra.Command, args []string) error { - if len(args) > 0 && (args[0] == cobra.ShellCompRequestCmd || args[0] == cobra.ShellCompNoDescRequestCmd) { + cmd, name, corrected, before := reorder(root, args) + if corrected == nil { return nil } - cmd := root + + if slices.Contains(before, "--version") { + return &tfserr.Error{ + Category: tfserr.Config, + Message: fmt.Sprintf("--version belongs to tfscli itself, not to %q (run it as: tfscli --version)", name), + } + } + if err := unknownFlag(cmd, before); err != nil { + return err + } + return &tfserr.Error{ + Category: tfserr.Config, + Message: fmt.Sprintf("the command %q must come first, before its flags and arguments (run it as: %s)", + name, shellJoin(append([]string{root.Name()}, corrected...))), + } +} + +// reorder finds the command that args name, as checkFlagOrder describes, and +// returns it with its name as typed; for a shell completion request, cmd is +// the root, whose flags are the ones that can precede it. When the path does +// not occupy the first words, corrected is args with the path moved to the +// front and every other word kept in its original order, and before holds +// the words that preceded the last word of the path; otherwise both are nil. +// +// Moving the path to the front can turn a word that preceded one of its steps +// into a further step, as "login" in "tfscli - login auth", so the move is +// repeated until the path stays where it is; it lengthens the path each time. +func reorder(root *cobra.Command, args []string) (cmd *cobra.Command, name string, corrected, before []string) { + cmd, name, corrected, before = moveFirst(root, args) + for corrected != nil { + next, nextName, again, _ := moveFirst(root, corrected) + if again == nil { + return next, nextName, corrected, before + } + corrected = again + } + return cmd, name, nil, nil +} + +// moveFirst makes one pass of reorder. before holds the words that preceded +// the last word of the path in args. +func moveFirst(root *cobra.Command, args []string) (cmd *cobra.Command, name string, corrected, before []string) { + cmd = root + words := []string{root.Name()} var path []int for i, arg := range args { - if arg == "--" { + if cmd == root && (arg == cobra.ShellCompRequestCmd || arg == cobra.ShellCompNoDescRequestCmd) { + words = append(words, arg) + path = append(path, i) break } if next := subcommand(cmd, arg); next != nil { cmd = next + words = append(words, arg) path = append(path, i) } } + name = strings.Join(words, " ") if len(path) == 0 || path[len(path)-1] == len(path)-1 { - return nil + return cmd, name, nil, nil } - // The corrected command is the path followed by every other word in its - // original order; before holds the words that preceded the last path word. last := path[len(path)-1] - var before, rest []string + corrected = make([]string, 0, len(args)) + var rest []string for i, arg := range args { if slices.Contains(path, i) { + corrected = append(corrected, arg) continue } if i < last { @@ -59,22 +108,7 @@ func checkFlagOrder(root *cobra.Command, args []string) error { } rest = append(rest, arg) } - - if slices.Contains(before, "--version") { - return &tfserr.Error{ - Category: tfserr.Config, - Message: fmt.Sprintf("--version belongs to tfscli itself, not to %q (run it as: tfscli --version)", cmd.CommandPath()), - } - } - if err := unknownFlag(cmd, before); err != nil { - return err - } - words := append(strings.Fields(cmd.CommandPath()), rest...) - return &tfserr.Error{ - Category: tfserr.Config, - Message: fmt.Sprintf("the command %q must come first, before its flags and arguments (run it as: %s)", - cmd.CommandPath(), shellJoin(words)), - } + return cmd, name, append(corrected, rest...), before } // subcommand returns the child of cmd named name, or nil. diff --git a/internal/cli/flagorder_test.go b/internal/cli/flagorder_test.go index f520c1a..0cdfa18 100644 --- a/internal/cli/flagorder_test.go +++ b/internal/cli/flagorder_test.go @@ -1,9 +1,18 @@ package cli import ( + "fmt" + "io" + "math/rand/v2" "net/http" + "regexp" + "runtime" + "slices" "strings" + "sync" "testing" + + "github.com/spf13/cobra" ) // refused is the error for a command line whose command path does not come @@ -110,6 +119,21 @@ func TestFlagBeforeTheCommandPathIsRefused(t *testing.T) { args: []string{"--verbose", "completion", "powershell"}, want: refused("tfscli completion powershell", "tfscli completion powershell --verbose"), }, + { + name: "flag before a shell completion request", + args: []string{"--verbose", "__complete", "wit", ""}, + want: refused("tfscli __complete", "tfscli __complete --verbose wit ''"), + }, + { + name: "double dash taken for the value of a flag", + args: []string{"--api-version", "--", "completion", "bash"}, + want: refused("tfscli completion bash", "tfscli completion bash --api-version --"), + }, + { + name: "path words in reverse order", + args: []string{"-", "login", "auth"}, + want: refused("tfscli auth login", "tfscli auth login -"), + }, { name: "version flag before a path", args: []string{"--version", "wit"}, @@ -214,3 +238,165 @@ func TestShellCompletionRequest(t *testing.T) { }) } } + +// The two tests above compare the check with expectations written by hand, +// which miss the lines where cobra resolves a command differently from what +// the expectations assume. Here cobra itself is the oracle, over every line +// up to three words long built from the words that matter to it and a +// fixed-seed sample of longer lines: +// +// - a line the check accepts makes cobra select a command whose path +// occupies the first words of the line; +// - the corrected line proposed for a misplaced path is accepted by the +// check, and cobra selects for it the command the message names. +func TestFlagOrderAgreesWithCobra(t *testing.T) { + // On Windows, cobra asks before every execution whether the process was + // started from Explorer, by listing every process of the system; at + // several milliseconds a call, that alone would make the test take + // minutes. An empty help text turns the question off. + mousetrap := cobra.MousetrapHelpText + cobra.MousetrapHelpText = "" + t.Cleanup(func() { cobra.MousetrapHelpText = mousetrap }) + + words := oracleWords() + var lines [][]string + var build func(line []string) + build = func(line []string) { + lines = append(lines, line) + if len(line) == 3 { + return + } + for _, w := range words { + build(append(slices.Clip(line), w)) + } + } + build(nil) + rng := rand.New(rand.NewPCG(1, 2)) + for range 20000 { + line := make([]string, 4+rng.IntN(4)) + for i := range line { + line[i] = words[rng.IntN(len(words))] + } + lines = append(lines, line) + } + + failures := make(chan string, len(lines)) + next := make(chan []string) + var wg sync.WaitGroup + for range runtime.GOMAXPROCS(0) { + wg.Go(func() { + for line := range next { + if msg := checkAgainstCobra(line); msg != "" { + failures <- msg + } + } + }) + } + for _, line := range lines { + next <- line + } + close(next) + wg.Wait() + close(failures) + + reported := 0 + for msg := range failures { + if reported++; reported > 20 { + t.Error("further failures omitted") + break + } + t.Error(msg) + } +} + +// checkAgainstCobra checks one command line against the two properties of +// TestFlagOrderAgreesWithCobra and describes the first one it breaks. +func checkAgainstCobra(line []string) string { + shown := shellJoin(append([]string{"tfscli"}, line...)) + err := checkFlagOrder(stubbedRoot(), line) + if err == nil { + cmd := selected(line) + if path := strings.Fields(cmd.CommandPath())[1:]; len(path) > len(line) || !slices.Equal(path, line[:len(path)]) { + return fmt.Sprintf("%s: accepted, but cobra selects %q", shown, cmd.CommandPath()) + } + return "" + } + if !strings.Contains(err.Error(), "must come first") { + return "" + } + _, name, corrected, _ := reorder(stubbedRoot(), line) + fixed := shellJoin(append([]string{"tfscli"}, corrected...)) + if err := checkFlagOrder(stubbedRoot(), corrected); err != nil { + return fmt.Sprintf("%s: corrected to %s, which is refused: %v", shown, fixed, err) + } + if got := selected(corrected); got.CommandPath() != name { + return fmt.Sprintf("%s: corrected to %s, for which cobra selects %q instead of %q", shown, fixed, got.CommandPath(), name) + } + return "" +} + +// selected returns the command cobra selects for args, whether it then runs +// the command, prints its help, or rejects its flags or arguments. +func selected(args []string) *cobra.Command { + root := stubbedRoot() + root.SetArgs(args) + cmd, _ := root.ExecuteC() + return cmd +} + +// stubbedRoot is the command tree with every action replaced by one that does +// nothing, so that cobra can execute any command line without a server. +func stubbedRoot() *cobra.Command { + root := newRoot(testBuild, noTerminal{}, io.Discard, io.Discard) + var stub func(cmd *cobra.Command) + stub = func(cmd *cobra.Command) { + cmd.Run = nil + cmd.RunE = func(*cobra.Command, []string) error { return nil } + for _, child := range cmd.Commands() { + stub(child) + } + } + stub(root) + return root +} + +// oracleWords is the alphabet of the generated command lines: every command +// name and every flag of the tree, a value flag also with its value attached, +// and the words that pflag and cobra treat specially. +func oracleWords() []string { + words := []string{ + "--", "-", "", "--help", "-h", "--version", "--bogus", "-z", "X", "5", + cobra.ShellCompRequestCmd, + } + var walk func(cmd *cobra.Command) + walk = func(cmd *cobra.Command) { + if cmd.HasParent() { + words = append(words, cmd.Name()) + } + for _, m := range flagUsage.FindAllStringSubmatch(cmd.LocalFlags().FlagUsages(), -1) { + short, name := m[1], m[2] + _, value := lookupFlag(cmd, name, false) + words = append(words, "--"+name) + if value { + words = append(words, "--"+name+"=X") + } + if short != "" { + words = append(words, "-"+short) + if value { + words = append(words, "-"+short+"X") + } + } + } + for _, child := range cmd.Commands() { + walk(child) + } + } + walk(newRoot(testBuild, noTerminal{}, io.Discard, io.Discard)) + slices.Sort(words) + return slices.Compact(words) +} + +// flagUsage matches the start of a line of cobra's flag usage text, such as +// " -p, --project string" or " --verbose", capturing the shorthand and +// the name. +var flagUsage = regexp.MustCompile(`(?m)^\s+(?:-(\w), )?--([\w-]+)`) From 9a7b9a5a3c076ab86e188c9d105b99c62854547d Mon Sep 17 00:00:00 2001 From: Dmitry Pleshakov Date: Fri, 9 Oct 2026 09:27:57 +0300 Subject: [PATCH 15/25] Fix the flag order findings of the branch review - Read the words before the path against the final path: when the path was found in more than one pass, flags before it went unchecked, and "tfscli work-items wit --bogus get 5" was told to move --bogus after the path instead of being told it is unknown. - Read "--" before the path as pflag does, as the end of the flags rather than as an unknown flag named "--"; report "--=x" and "---x" as bad flag syntax; read a group of shorthands letter by letter; and refuse --version only where pflag would read it as a flag. - Refuse cobra's case-insensitive command matching in the tree test. - Extend the oracle test to refusals for a flag before the path, give each worker one tree, keep completion requests from writing cobra's diagnostics to stderr, and report failures in a stable order; turn off cobra's Explorer check for the whole package in TestMain. - Narrow the group rule in CLAUDE.md and CONTRIBUTING.md to areas, resources, and local groups; describe the quoting, the reading of the words before the path, and the oracle in docs/architecture.md. - Merge the changelog entry on --time-precision into the entry on the flag order; in README, put the opencode deny rule last and state that the rules govern the usual form of a call, not a determined agent. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01XA4Hx4kZtgobXtNCEpuiTC --- CHANGELOG.md | 3 +- CLAUDE.md | 2 +- CONTRIBUTING.md | 5 +- README.md | 15 +-- docs/architecture.md | 2 +- docs/project-brief.md | 2 +- internal/cli/cli_test.go | 12 +++ internal/cli/flagorder.go | 188 ++++++++++++++++++++------------- internal/cli/flagorder_test.go | 166 ++++++++++++++++++++++------- internal/cli/tree_test.go | 7 +- 10 files changed, 275 insertions(+), 127 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c371b2..c08fafc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,10 +8,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Changed -- A flag placed before the end of the command path, such as `--verbose` in `tfscli --verbose wit work-items get 12345` or `--help` in `tfscli --help wit work-items get`, is now refused with a `config` error that shows the command with the flag moved after it, where it used to be accepted or to fail with a misleading "unknown command"; flags now always follow the command, so that agent permission rules can match the command by its name. +- A flag placed before the end of the command path, such as `--verbose` in `tfscli --verbose wit work-items get 12345` or `--help` in `tfscli --help wit work-items get`, is now refused with a `config` error that shows the command with the flag moved after it, where it used to be accepted, to fail with a misleading "unknown command", or, for a flag of an action placed before the action, as in `tfscli wit wiql --time-precision query-by-wiql`, to print the help of the group and exit successfully without running the query; flags now always follow the command, so that agent permission rules can match the command by its name. ### Fixed -- A flag of an action placed before the action, as in `tfscli wit wiql --time-precision query-by-wiql`, no longer makes tfscli print the help of the group and exit successfully without running the query. - An unknown command under `tfscli auth`, such as `tfscli auth logn`, is now reported as an unknown command, where tfscli used to print the help of `auth` and exit successfully. ## [0.1.1] - 2026-10-06 diff --git a/CLAUDE.md b/CLAUDE.md index 7291ed7..f2b1367 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -23,7 +23,7 @@ These come from `project-brief.md` and override casual feature requests. If a ch - **PAT auth only.** No SSPI or NTLM. A PAT is issued for one collection, so the server URL, the collection, and the PAT are stored together and always come from one source: `$XDG_DATA_HOME/tfscli/auth.json` (default `~/.local/share/tfscli/auth.json`, mode `0600`, written only by the interactive `tfscli auth login`) or `TFSCLI_AUTH` holding the same JSON, which takes precedence. No flag, separate env var, or config key may supply the URL, the collection, or the token. No OS keychain unless an organisation requires one. - **Read-only in v1.** Write operations are explicitly out of scope. They additionally depend on JSON output landing first, because markdown is lossy. - **Write operations are gated on the command line.** When they are added, every operation that changes server state requires `--allow-changes`, and an irreversible one (its effect cannot be undone by another API operation) requires `--allow-irreversible` as well. Writing is a property of the command, never of a flag of a reading command. The gate flags have no short form, environment variable, or config key, and are checked before any request. See "Write operations" in Non-Goals of `project-brief.md`. -- **The command line stays matchable by agent permission rules.** Flags follow the last word of the command path; each command has one form (no aliases, no abbreviations, no prefix matching); a command with subcommands only groups them (no arguments, no flags of its own, nothing but its help); local command names never coincide with REST API areas; no generic pass-through command. See "The command line can be governed by permission rules" in `project-brief.md`. +- **The command line stays matchable by agent permission rules.** Flags follow the last word of the command path; each command has one form (no aliases, no abbreviations, no prefix or case-insensitive matching); a group of commands (an area, a resource, or a local group such as `auth`) only groups them (no arguments, no flags of its own, nothing but its help); local command names never coincide with REST API areas; no generic pass-through command. See "The command line can be governed by permission rules" in `project-brief.md`. - **Markdown is the default output.** HTML fields (Description, ReproSteps, etc.) are converted via `github.com/JohannesKaufmann/html-to-markdown/v2`. JSON output is a planned `--json` flag, not present in v1. - **Mirror TFS REST API structure.** Command hierarchy, parameter names, and behaviour follow the API. Any deviation needs explicit usability justification — don't invent new abstractions (no custom query syntax, no synthetic batch endpoints, no caching). - **Stateless.** No daemon, no background process, no on-disk cache. Every call hits the server. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 603a808..a3e911c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -83,8 +83,9 @@ if a change would break one, raise it before implementing. - The command hierarchy and parameter names mirror the TFS REST API; a deviation needs an explicit usability justification, and no query syntax or batch endpoint is invented. - The command line stays matchable by agent permission rules: flags follow the command - path, each command has one form (no aliases, abbreviations, or prefix matching), a - command with subcommands only groups them (no arguments, no flags of its own), local + path, each command has one form (no aliases, abbreviations, or prefix or + case-insensitive matching), a group of commands (an area, a resource, or a local group + such as `auth`) only groups them (no arguments, no flags of its own), local command names never coincide with REST API areas, and there is no generic pass-through command. - Stateless: no daemon, no background process, no on-disk state beyond the config file and the credential written by `auth login`. diff --git a/README.md b/README.md index 9eb0b69..84d8f09 100644 --- a/README.md +++ b/README.md @@ -163,8 +163,8 @@ covers the command whatever its flags. The current commands only read; Commands that change data are not part of tfscli yet. When they are added, each will require the flag `--allow-changes` and will fall under `tfscli wit *` -like the reading commands, so the examples below already ask before any -command carrying that flag. +like the reading commands, so the examples below already ask before a command +that carries that flag as it is usually written. Claude Code, in `.claude/settings.json` or `~/.claude/settings.json`; the `PowerShell` rules apply where the agent runs commands through PowerShell. An @@ -200,8 +200,8 @@ any catch-all `"*"` rule, in this order: "bash": { "tfscli wit *": "allow", "tfscli licenses": "allow", - "tfscli auth *": "deny", - "*--allow-changes*": "ask" + "*--allow-changes*": "ask", + "tfscli auth *": "deny" } } } @@ -210,8 +210,11 @@ any catch-all `"*"` rule, in this order: A rule matches the command as the agent types it. A call through `tfscli.exe` or a full path matches none of the `tfscli` rules and gets the agent's default for unmatched commands: Claude Code asks in its default mode, while opencode -allows it unless a catch-all `"*": "ask"` is set. The deny rule therefore keeps -an agent from the usual form of the call; it is not a security boundary. +allows it unless a catch-all `"*": "ask"` is set. Likewise, a flag written +with quotes in the middle, such as `--allow-chan''ges`, reaches tfscli as +`--allow-changes` but may not match the rule. The rules therefore govern the +usual form of a call and guard against an agent's mistake; they are not a +security boundary against an agent that sets out to get around them. ## Configuration diff --git a/docs/architecture.md b/docs/architecture.md index 6f66d77..38a78ba 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -195,7 +195,7 @@ This section describes the conceptual module structure, their responsibilities, ### Modules and responsibilities -- **cli** — cobra commands, nested as area, resource, and action after the REST API reference (`wit` → `work-items` → `get`); persistent flags (`--verbose`, config overrides); orchestrates the chain config → apiclient → domain → printer → exit. Owns the markdown printer functions (one per resource and shape, such as a single work item and several) plus small per-kind scalar formatters for identity and datetime values used by those printers. No `Renderer` interface in v1 — printers are plain functions. When `--json` is added later, the interface and a second renderer can be introduced here without touching the domain layer. Enforces the command line shape that "The command line can be governed by permission rules" in `project-brief.md` requires. Cobra accepts flags at any position, so `run` checks the arguments before executing the tree: the path of the command must occupy exactly the first words of the command line, and otherwise the command fails with a `config` error that shows the corrected command — the path followed by every other word in its original order, quoted for the shell in single quotes, which bash and PowerShell read alike. The command is the one reached from the root by taking, in order, every word that names a subcommand of the command reached so far; flag values are not read, and `--` does not stop the search, since pflag takes it for the value of a flag placed before it. When moving the path to the front turns a word that preceded one of its steps into a further step, as `login` in `tfscli - login auth`, the move is repeated, so that the corrected command is itself accepted. This depends on the shape of the tree: a command with subcommands takes no arguments and defines no flags of its own, so in a valid command line no word naming a subcommand can follow the path, and the rule reaches at least as deep as cobra's `Find`, which descends only through such words. A rule that read flag values to find the path was rejected: it is a second parser of the command line, and each of its divergences from pflag let a command run with its path not at the start. `newRoot` registers cobra's `help` and `completion` commands up front so that they are found like any other command. Cobra's hidden `__complete` command, which the shell completion scripts run, is added only when cobra executes the tree, so the check takes it for a subcommand of the root that ends the path: the words after it are the command line to complete. A flag before the path that the command does not define is reported as unknown, in pflag's wording, rather than moved into a corrected command that would fail the same way; `--version`, defined on the root alone, is refused before a path with a message that names `tfscli --version`. The check is explicit rather than a root `PersistentPreRunE`, because cobra runs only the nearest such hook and a child command defining its own would silently drop it. A test walks the whole command tree and fails on any command alias, on cobra's prefix matching, and on a command with subcommands that accepts an argument or defines a flag of its own, so that each command keeps exactly one form and the check keeps the shape it relies on. Another test takes cobra as the oracle: over every command line of up to three words built from the names and flags of the tree and the words pflag treats specially, and a fixed-seed sample of longer lines, it requires that a line the check accepts makes cobra select a command whose path opens the line, and that the corrected command proposed for a refused line is accepted and makes cobra select the command the message names. Every group, local ones included, is built with `newGroupCmd`. +- **cli** — cobra commands, nested as area, resource, and action after the REST API reference (`wit` → `work-items` → `get`); persistent flags (`--verbose`, config overrides); orchestrates the chain config → apiclient → domain → printer → exit. Owns the markdown printer functions (one per resource and shape, such as a single work item and several) plus small per-kind scalar formatters for identity and datetime values used by those printers. No `Renderer` interface in v1 — printers are plain functions. When `--json` is added later, the interface and a second renderer can be introduced here without touching the domain layer. Enforces the command line shape that "The command line can be governed by permission rules" in `project-brief.md` requires. Cobra accepts flags at any position, so `run` checks the arguments before executing the tree: the path of the command must occupy exactly the first words of the command line, and otherwise the command fails with a `config` error that shows the corrected command — the path followed by every other word in its original order, quoted for the shell in single quotes, which bash and PowerShell read alike, or in double quotes for a word that itself contains a single quote. The command is the one reached from the root by taking, in order, every word that names a subcommand of the command reached so far; flag values are not read, and `--` does not stop the search, since pflag takes it for the value of a flag placed before it. When moving the path to the front turns a word that preceded one of its steps into a further step, as `login` in `tfscli - login auth`, the move is repeated, so that the corrected command is itself accepted. This depends on the shape of the tree: a group takes no arguments and defines no flags of its own, and the flags cobra adds itself, `--help` and the root's `--version`, take no value, so in a valid command line no word naming a subcommand can follow the path, and the rule reaches at least as deep as cobra's `Find`, which descends only through such words. A rule that read flag values to find the path was rejected: it is a second parser of the command line, and each of its divergences from pflag let a command run with its path not at the start. `newRoot` registers cobra's `help` and `completion` commands up front so that they are found like any other command. Cobra's hidden `__complete` command, which the shell completion scripts run, is added only when cobra executes the tree, so the check takes it for a subcommand of the root that ends the path: the words after it are the command line to complete. A flag before the path that the command does not define, or that is malformed, is reported as such, in pflag's wording, rather than moved into a corrected command that would fail the same way; the words before the path are read as pflag reads them, so a flag's value and the words after `--` are not taken for flags. `--version`, defined on the root alone, is refused before a path with a message that names `tfscli --version`. The check is explicit rather than a root `PersistentPreRunE`, because cobra runs only the nearest such hook and a child command defining its own would silently drop it. A test walks the whole command tree and fails on any command alias, on cobra's prefix or case-insensitive matching, and on a command with subcommands that accepts an argument or defines a flag of its own, so that each command keeps exactly one form and the check keeps the shape it relies on. Another test takes cobra as the oracle: over every command line of up to three words built from the names and flags of the tree and the words pflag treats specially, and a fixed-seed sample of longer lines, it requires that a line the check accepts makes cobra select a command whose path opens the line, that the corrected command proposed for a refused line is accepted, makes cobra select the command the message names, and leaves cobra no flag before the path to reject, and that a line refused for a flag before the path is rejected by cobra with the same message once the path is moved to the front. The tests turn off cobra's check, on Windows, of whether the process was started from Explorer: it lists every process of the system before each execution, which would make this test take minutes. Every group, local ones included, is built with `newGroupCmd`. - **config** — resolves the credential (`URL`, `Collection`, and `PAT`; a PAT is issued for one collection) from `TFSCLI_AUTH` or `$XDG_DATA_HOME/tfscli/auth.json`, never from a mix of sources; loads the optional `$XDG_CONFIG_HOME/tfscli/config.json`, applies environment overrides (`TFSCLI_*`), applies cobra flag overrides; validates that all required fields are set for the command being run. Writes `auth.json` for `auth login` (mode `0600`, directory `0700`). Holds `URL`, `Collection`, `PAT`, `Project`, `APIVersion`, `InsecureSkipVerify`, `CABundle`. Never logs PAT. - **apiclient** — wraps `net/http`. Builds URLs from `Config.URL + Collection + path` and adds `api-version`. A configured `APIVersion` is sent as it is. Otherwise the version is negotiated per request, as the official SDKs do: `OPTIONS {URL}/{Collection}/_apis` lists every resource location with its versions, the caller names its location by id, and the request carries the location's released version, or `{maxVersion}-preview.{resourceVersion}` when it has none — the version the server chooses for a request without one. The response is not cached. A 401 or a transport failure on `OPTIONS` fails the request; any other failure sends it without a version and notes why through `Logger.Info`, and a refusal of that request for the missing version gets the settings to use in its message. A call without a location, the login check, neither negotiates nor sends a version. Sets `Authorization: Basic base64(":")` on every request. Configures TLS using `CABundle` (appended to system root pool) and `InsecureSkipVerify`. Hooks the logger via a `RoundTripper` — that transport is the single call site of `LogRequest`, so every outgoing request is logged exactly once and no other module logs HTTP traffic. Classifies HTTP outcomes into the stable error categories defined in the brief (`auth`, `not_found`, `forbidden`, `server`, `config`, `network`). - **workitem** — domain logic for Work Items: Get Work Item, Work Items - List, and Get Work Items Batch. Owns the `WorkItem`/`Field` types, the `BatchRequest`/`Batch` types for reading several work items, and the allowlist of fields known to contain HTML (`System.Description`, `Microsoft.VSTS.TCM.ReproSteps`, `Microsoft.VSTS.TCM.SystemInfo`, `Microsoft.VSTS.Common.AcceptanceCriteria`, …). After unmarshalling, tags each field with its `FieldKind`. For several work items, keeps the server's order and reports the requested ids the server did not return. Returns raw values — does not perform markdown conversion. diff --git a/docs/project-brief.md b/docs/project-brief.md index 585955f..d15703c 100644 --- a/docs/project-brief.md +++ b/docs/project-brief.md @@ -62,7 +62,7 @@ An AI agent can call `tfscli`, read work item content, and use it meaningfully i - A query or body parameter is a flag with the API name in kebab-case and without a leading `$`: `ids` is `--ids`, `errorPolicy` is `--error-policy`, `$expand` is `--expand`. A flag that is not given is not sent, so the server's default applies. - **The command line can be governed by permission rules.** AI agent hosts, such as Claude Code and opencode, decide whether an agent may run a command by matching its text against allow, ask, and deny rules. tfscli keeps that text predictable: - The command path opens the command line, and flags follow its last word: `tfscli wit work-items get --verbose 12345`, not `tfscli --verbose wit work-items get 12345`. A flag placed earlier is refused with a `config` error that shows the corrected command. The rule has no exceptions: `--help` follows the path like any other flag, and the built-in `help` and `completion` commands are commands like any other. Only an invocation without a command path, such as `tfscli --help` or `tfscli --version`, has no path for a flag to precede; `--version` belongs to the root alone. - - Each command has exactly one form: no command aliases, no abbreviated command or flag names, no prefix matching of command names. + - Each command has exactly one form: no command aliases, no abbreviated command or flag names, no prefix or case-insensitive matching of command names. - A command that has subcommands — an area, a resource, or a local group such as `auth` — only groups them: it takes no arguments, defines no flags of its own, and does nothing but print its help. Only a command without subcommands runs. A group that also acts on its own, as `git stash` and `git remote` do, is excluded: a rule written for the group would also cover every command below it, and the command path could no longer be told from the group's own arguments. - The names of local commands (`auth`, `licenses`, `help`, `completion`) never coincide with an area of the REST API reference. - Whether a command changes server state is a property of the command, never of its flags: a reading command does not gain a side effect through a flag. There is no generic pass-through command that sends an arbitrary request; should one be added, it falls under the gate flags described in "Non-Goals". diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 808a6c5..9e3c946 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -12,8 +12,20 @@ import ( "path/filepath" "strings" "testing" + + "github.com/spf13/cobra" ) +// TestMain turns off a question cobra asks on Windows before every execution: +// whether the process was started from Explorer, answered by listing every +// process of the system. At several milliseconds a call, it would make the +// tests that execute the command tree many thousands of times, such as +// TestFlagOrderAgreesWithCobra, take minutes. +func TestMain(m *testing.M) { + cobra.MousetrapHelpText = "" + m.Run() +} + // workItemResponse is a Get Work Item response trimmed to the fields the // printer has to distinguish. The full-response case lives in the workitem // package; here it only has to reach stdout intact. diff --git a/internal/cli/flagorder.go b/internal/cli/flagorder.go index bf2e601..d31c6b4 100644 --- a/internal/cli/flagorder.go +++ b/internal/cli/flagorder.go @@ -17,13 +17,14 @@ import ( // has to open the command line; cobra itself accepts flags at any position. // // The command is the one reached from the root by taking, in order, every word -// that names a subcommand of the command reached so far, "--" not excepted, -// since pflag takes it for the value of a flag before it. No -// knowledge of flag values is needed: a command with subcommands takes no -// arguments and defines no flags of its own (TestEveryGroupOnlyGroups), so in -// a valid command line no word naming a subcommand can follow the path. This -// reaches at least as deep as cobra's Find, which descends only through such -// words, so cobra never runs a command whose path is not at the start. +// that names a subcommand of the command reached so far. The walk does not +// stop at "--", since pflag takes it for the value of a flag placed before it. +// No knowledge of flag values is needed: a group takes no arguments and +// defines no flags of its own (TestEveryGroupOnlyGroups), and the flags cobra +// adds itself, --help and the root's --version, take no value, so in a valid +// command line no word naming a subcommand can follow the path. This reaches +// at least as deep as cobra's Find, which descends only through such words, +// so cobra never runs a command whose path is not at the start. // // Cobra's hidden command for shell completion requests, which it adds to the // root only when it executes the tree, is taken as a subcommand of the root @@ -34,13 +35,7 @@ func checkFlagOrder(root *cobra.Command, args []string) error { return nil } - if slices.Contains(before, "--version") { - return &tfserr.Error{ - Category: tfserr.Config, - Message: fmt.Sprintf("--version belongs to tfscli itself, not to %q (run it as: tfscli --version)", name), - } - } - if err := unknownFlag(cmd, before); err != nil { + if err := unknownFlag(cmd, name, before); err != nil { return err } return &tfserr.Error{ @@ -55,30 +50,60 @@ func checkFlagOrder(root *cobra.Command, args []string) error { // the root, whose flags are the ones that can precede it. When the path does // not occupy the first words, corrected is args with the path moved to the // front and every other word kept in its original order, and before holds -// the words that preceded the last word of the path; otherwise both are nil. +// the words of args that preceded the last word of the path; otherwise both +// are nil. // // Moving the path to the front can turn a word that preceded one of its steps // into a further step, as "login" in "tfscli - login auth", so the move is // repeated until the path stays where it is; it lengthens the path each time. func reorder(root *cobra.Command, args []string) (cmd *cobra.Command, name string, corrected, before []string) { - cmd, name, corrected, before = moveFirst(root, args) - for corrected != nil { - next, nextName, again, _ := moveFirst(root, corrected) - if again == nil { - return next, nextName, corrected, before + // order holds the indices in args of the words of the line being read. + order := make([]int, len(args)) + for i := range order { + order[i] = i + } + var path []int + for { + line := make([]string, len(order)) + for i, o := range order { + line[i] = args[o] + } + cmd, name, path = findPath(root, line) + if len(path) == 0 || path[len(path)-1] == len(path)-1 { + if slices.IsSorted(order) { + return cmd, name, nil, nil + } + corrected = line + break + } + next := make([]int, 0, len(order)) + for _, p := range path { + next = append(next, order[p]) + } + for i, o := range order { + if !slices.Contains(path, i) { + next = append(next, o) + } } - corrected = again + order = next } - return cmd, name, nil, nil + + last := slices.Max(order[:len(path)]) + for _, o := range order[len(path):] { + if o < last { + before = append(before, args[o]) + } + } + return cmd, name, corrected, before } -// moveFirst makes one pass of reorder. before holds the words that preceded -// the last word of the path in args. -func moveFirst(root *cobra.Command, args []string) (cmd *cobra.Command, name string, corrected, before []string) { +// findPath walks line from the root as checkFlagOrder describes and returns +// the command reached, its name as typed, and the indices in line of the +// words of its path. +func findPath(root *cobra.Command, line []string) (cmd *cobra.Command, name string, path []int) { cmd = root words := []string{root.Name()} - var path []int - for i, arg := range args { + for i, arg := range line { if cmd == root && (arg == cobra.ShellCompRequestCmd || arg == cobra.ShellCompNoDescRequestCmd) { words = append(words, arg) path = append(path, i) @@ -90,25 +115,7 @@ func moveFirst(root *cobra.Command, args []string) (cmd *cobra.Command, name str path = append(path, i) } } - name = strings.Join(words, " ") - if len(path) == 0 || path[len(path)-1] == len(path)-1 { - return cmd, name, nil, nil - } - - last := path[len(path)-1] - corrected = make([]string, 0, len(args)) - var rest []string - for i, arg := range args { - if slices.Contains(path, i) { - corrected = append(corrected, arg) - continue - } - if i < last { - before = append(before, arg) - } - rest = append(rest, arg) - } - return cmd, name, append(corrected, rest...), before + return cmd, strings.Join(words, " "), path } // subcommand returns the child of cmd named name, or nil. @@ -122,44 +129,77 @@ func subcommand(cmd *cobra.Command, name string) *cobra.Command { } // unknownFlag reports the first flag among words, read as pflag reads them, -// that cmd does not define, in pflag's own wording. Moving such a flag after -// the path would not make the command valid, so it is reported as unknown -// rather than as misplaced. -func unknownFlag(cmd *cobra.Command, words []string) error { +// that cmd, named name, does not define or that is malformed, in pflag's own +// wording. Moving such a flag after the path would not make the command +// valid, so it is reported as it is rather than as misplaced. --version, +// which cobra defines on the root alone, is reported with the way to run it. +// Words after "--" are arguments, as they are to pflag. +func unknownFlag(cmd *cobra.Command, name string, words []string) error { for i := 0; i < len(words); i++ { word := words[i] - if word == "-" || !strings.HasPrefix(word, "-") { + switch { + case word == "--": + return nil + case word == "-" || !strings.HasPrefix(word, "-"): continue + case strings.HasPrefix(word, "--"): + body := word[2:] + if body[0] == '-' || body[0] == '=' { + return &tfserr.Error{Category: tfserr.Config, Message: "bad flag syntax: " + word} + } + flag, _, inline := strings.Cut(body, "=") + if flag == "help" { + continue + } + found, value := lookupFlag(cmd, flag, false) + switch { + case !found && flag == "version": + return &tfserr.Error{ + Category: tfserr.Config, + Message: fmt.Sprintf("--version belongs to tfscli itself, not to %q (run it as: tfscli --version)", name), + } + case !found: + return &tfserr.Error{Category: tfserr.Config, Message: "unknown flag: --" + flag} + } + if value && !inline { + i++ + } + default: + takesNext, err := shorthands(cmd, word[1:]) + if err != nil { + return err + } + if takesNext { + i++ + } } - name, short, inline := splitFlag(word) - if name == "help" && !short || name == "h" && short { + } + return nil +} + +// shorthands reads a group of shorthand flags, such as "v" in "-v" or "vpX" +// in "-vpX", as pflag reads it, and reports whether the last flag of the group +// takes its value from the next word. The help flag, which cobra adds only +// when it executes the tree, takes no value. +func shorthands(cmd *cobra.Command, group string) (takesNext bool, err error) { + for ; group != ""; group = group[1:] { + if group[0] == 'h' { continue } - found, value := lookupFlag(cmd, name, short) + found, value := lookupFlag(cmd, group[:1], true) switch { - case !found && short: - return &tfserr.Error{Category: tfserr.Config, Message: fmt.Sprintf("unknown shorthand flag: '%s' in %s", name, word)} case !found: - return &tfserr.Error{Category: tfserr.Config, Message: "unknown flag: --" + name} - case value && !inline: - i++ + return false, &tfserr.Error{ + Category: tfserr.Config, + Message: fmt.Sprintf("unknown shorthand flag: %q in -%s", group[0], group), + } + case len(group) > 1 && group[1] == '=': + return false, nil + case value: + return len(group) == 1, nil } } - return nil -} - -// splitFlag takes a flag token apart: "--name", "--name=value", "-n", or -// "-nvalue". inline reports whether the token carries its value. -func splitFlag(arg string) (name string, short, inline bool) { - if body, ok := strings.CutPrefix(arg, "--"); ok { - name, _, inline = strings.Cut(body, "=") - return name, false, inline - } - body := strings.TrimPrefix(arg, "-") - if len(body) > 1 { - return body[:1], true, true - } - return body, true, false + return false, nil } // lookupFlag finds a flag of cmd, its own or inherited, by name or by diff --git a/internal/cli/flagorder_test.go b/internal/cli/flagorder_test.go index 0cdfa18..f501d43 100644 --- a/internal/cli/flagorder_test.go +++ b/internal/cli/flagorder_test.go @@ -1,6 +1,7 @@ package cli import ( + "errors" "fmt" "io" "math/rand/v2" @@ -13,6 +14,8 @@ import ( "testing" "github.com/spf13/cobra" + + "github.com/dpleshakov/tfscli/internal/tfserr" ) // refused is the error for a command line whose command path does not come @@ -134,6 +137,31 @@ func TestFlagBeforeTheCommandPathIsRefused(t *testing.T) { args: []string{"-", "login", "auth"}, want: refused("tfscli auth login", "tfscli auth login -"), }, + { + name: "double dash before the path", + args: []string{"--", "wit", "work-items", "get", "5"}, + want: refused("tfscli wit work-items get", "tfscli wit work-items get -- 5"), + }, + { + name: "unknown flag before a path found in a second pass", + args: []string{"work-items", "wit", "--nope", "get", "5"}, + want: "Error [config]: unknown flag: --nope\n", + }, + { + name: "version flag before a path found in a second pass", + args: []string{"work-items", "wit", "--version", "get", "5"}, + want: `Error [config]: --version belongs to tfscli itself, not to "tfscli wit work-items get" (run it as: tfscli --version)` + "\n", + }, + { + name: "long flag without a name", + args: []string{"--=x", "wit", "work-items", "get", "5"}, + want: "Error [config]: bad flag syntax: --=x\n", + }, + { + name: "long flag with three dashes", + args: []string{"---x", "wit", "work-items", "get", "5"}, + want: "Error [config]: bad flag syntax: ---x\n", + }, { name: "version flag before a path", args: []string{"--version", "wit"}, @@ -239,25 +267,20 @@ func TestShellCompletionRequest(t *testing.T) { } } -// The two tests above compare the check with expectations written by hand, -// which miss the lines where cobra resolves a command differently from what -// the expectations assume. Here cobra itself is the oracle, over every line -// up to three words long built from the words that matter to it and a -// fixed-seed sample of longer lines: +// The tests above compare the check with expectations written by hand, which +// miss the lines where cobra resolves a command differently from what the +// expectations assume. Here cobra itself is the oracle, over every line up to +// three words long built from the words that matter to it and a fixed-seed +// sample of longer lines: // // - a line the check accepts makes cobra select a command whose path // occupies the first words of the line; // - the corrected line proposed for a misplaced path is accepted by the -// check, and cobra selects for it the command the message names. +// check, cobra selects for it the command the message names, and cobra +// rejects none of the flags that preceded the path; +// - a line refused for a flag before the path is rejected by cobra too, with +// the same message, once the path is moved to the front. func TestFlagOrderAgreesWithCobra(t *testing.T) { - // On Windows, cobra asks before every execution whether the process was - // started from Explorer, by listing every process of the system; at - // several milliseconds a call, that alone would make the test take - // minutes. An empty help text turns the question off. - mousetrap := cobra.MousetrapHelpText - cobra.MousetrapHelpText = "" - t.Cleanup(func() { cobra.MousetrapHelpText = mousetrap }) - words := oracleWords() var lines [][]string var build func(line []string) @@ -285,8 +308,10 @@ func TestFlagOrderAgreesWithCobra(t *testing.T) { var wg sync.WaitGroup for range runtime.GOMAXPROCS(0) { wg.Go(func() { + // The check only reads the tree, so one tree serves a worker. + root := newRoot(testBuild, noTerminal{}, io.Discard, io.Discard) for line := range next { - if msg := checkAgainstCobra(line); msg != "" { + if msg := checkAgainstCobra(root, line); msg != "" { failures <- msg } } @@ -299,49 +324,112 @@ func TestFlagOrderAgreesWithCobra(t *testing.T) { wg.Wait() close(failures) - reported := 0 + var msgs []string for msg := range failures { - if reported++; reported > 20 { - t.Error("further failures omitted") + msgs = append(msgs, msg) + } + slices.Sort(msgs) + for i, msg := range msgs { + if i == 20 { + t.Errorf("%d further failures omitted", len(msgs)-i) break } t.Error(msg) } } -// checkAgainstCobra checks one command line against the two properties of +// checkAgainstCobra checks one command line against the properties of // TestFlagOrderAgreesWithCobra and describes the first one it breaks. -func checkAgainstCobra(line []string) string { +func checkAgainstCobra(root *cobra.Command, line []string) string { shown := shellJoin(append([]string{"tfscli"}, line...)) - err := checkFlagOrder(stubbedRoot(), line) - if err == nil { - cmd := selected(line) - if path := strings.Fields(cmd.CommandPath())[1:]; len(path) > len(line) || !slices.Equal(path, line[:len(path)]) { - return fmt.Sprintf("%s: accepted, but cobra selects %q", shown, cmd.CommandPath()) + cmd, name, corrected, before := reorder(root, line) + err := checkFlagOrder(root, line) + switch { + case corrected == nil && err != nil: + return fmt.Sprintf("%s: refused although its path comes first: %v", shown, err) + case corrected == nil: + // Cobra serves a completion request by construction; executing it + // would only print cobra's completion diagnostics to the real stderr. + if len(line) > 0 && (line[0] == cobra.ShellCompRequestCmd || line[0] == cobra.ShellCompNoDescRequestCmd) { + return "" + } + got := selected(line) + if path := strings.Fields(got)[1:]; len(path) > len(line) || !slices.Equal(path, line[:len(path)]) { + return fmt.Sprintf("%s: accepted, but cobra selects %q", shown, got) } return "" + case err == nil: + return fmt.Sprintf("%s: accepted, but its path does not come first", shown) } - if !strings.Contains(err.Error(), "must come first") { - return "" + + var te *tfserr.Error + if !errors.As(err, &te) { + return fmt.Sprintf("%s: refused with an error of no category: %v", shown, err) } - _, name, corrected, _ := reorder(stubbedRoot(), line) fixed := shellJoin(append([]string{"tfscli"}, corrected...)) - if err := checkFlagOrder(stubbedRoot(), corrected); err != nil { + moved := append(slices.Clip(corrected[:len(strings.Fields(name))-1]), before...) + // A group ignores unknown flags, and the flags before a completion + // request belong to the root, which is a group too. + leaf := !cmd.HasSubCommands() + + if !strings.Contains(te.Message, "must come first") { + if !leaf { + return "" + } + switch got := flagError(moved); { + case got == "": + return fmt.Sprintf("%s: refused with %q, but cobra accepts the flags before the path", shown, te.Message) + case got != te.Message && !strings.HasPrefix(te.Message, "--version"): + return fmt.Sprintf("%s: refused with %q, but cobra rejects it with %q", shown, te.Message, got) + } + return "" + } + + if err := checkFlagOrder(root, corrected); err != nil { return fmt.Sprintf("%s: corrected to %s, which is refused: %v", shown, fixed, err) } - if got := selected(corrected); got.CommandPath() != name { - return fmt.Sprintf("%s: corrected to %s, for which cobra selects %q instead of %q", shown, fixed, got.CommandPath(), name) + if cmd != root { + if got := selected(corrected); got != name { + return fmt.Sprintf("%s: corrected to %s, for which cobra selects %q instead of %q", shown, fixed, got, name) + } + } + if !leaf { + return "" + } + if got := flagError(moved); got != "" { + return fmt.Sprintf("%s: corrected to %s, though cobra rejects a flag before the path: %s", shown, fixed, got) } return "" } -// selected returns the command cobra selects for args, whether it then runs -// the command, prints its help, or rejects its flags or arguments. -func selected(args []string) *cobra.Command { +// selected returns the path, as typed, of the command cobra selects for args, +// whether it then runs the command, prints its help, or rejects its flags or +// arguments. +func selected(args []string) string { root := stubbedRoot() root.SetArgs(args) cmd, _ := root.ExecuteC() - return cmd + if cmd.HasParent() && cmd.CalledAs() != "" { + return cmd.Parent().CommandPath() + " " + cmd.CalledAs() + } + return cmd.CommandPath() +} + +// flagError returns the message with which cobra rejects a flag of args as +// unknown or malformed, or "" if it rejects none. +func flagError(args []string) string { + root := stubbedRoot() + root.SetArgs(args) + _, err := root.ExecuteC() + if err == nil { + return "" + } + for _, kind := range []string{"unknown flag: ", "unknown shorthand flag: ", "bad flag syntax: "} { + if strings.HasPrefix(err.Error(), kind) { + return err.Error() + } + } + return "" } // stubbedRoot is the command tree with every action replaced by one that does @@ -365,8 +453,8 @@ func stubbedRoot() *cobra.Command { // and the words that pflag and cobra treat specially. func oracleWords() []string { words := []string{ - "--", "-", "", "--help", "-h", "--version", "--bogus", "-z", "X", "5", - cobra.ShellCompRequestCmd, + "--", "-", "", "--=X", "--help", "-h", "--version", "--bogus", "-z", "X", "5", + cobra.ShellCompRequestCmd, cobra.ShellCompNoDescRequestCmd, } var walk func(cmd *cobra.Command) walk = func(cmd *cobra.Command) { @@ -398,5 +486,7 @@ func oracleWords() []string { // flagUsage matches the start of a line of cobra's flag usage text, such as // " -p, --project string" or " --verbose", capturing the shorthand and -// the name. +// the name. The flags are read from the usage text because visiting the flag +// set would name pflag's Flag type and so make pflag a direct dependency; +// a hidden flag is not listed there and therefore not exercised. var flagUsage = regexp.MustCompile(`(?m)^\s+(?:-(\w), )?--([\w-]+)`) diff --git a/internal/cli/tree_test.go b/internal/cli/tree_test.go index bd607c4..5bdc116 100644 --- a/internal/cli/tree_test.go +++ b/internal/cli/tree_test.go @@ -8,12 +8,15 @@ import ( ) // Agent permission rules match the command text, so each command must have -// exactly one form: an alias or prefix matching would let a command run under -// a name no rule was written for. +// exactly one form: an alias, prefix matching, or case-insensitive matching +// would let a command run under a name no rule was written for. func TestEveryCommandHasOneForm(t *testing.T) { if cobra.EnablePrefixMatching { t.Error("cobra.EnablePrefixMatching is on; command names must be matched in full") } + if cobra.EnableCaseInsensitive { + t.Error("cobra.EnableCaseInsensitive is on; command names must be matched as written") + } var walk func(cmd *cobra.Command) walk = func(cmd *cobra.Command) { From 5d1de678782e477304ff35579196ec2c111ff54f Mon Sep 17 00:00:00 2001 From: Dmitry Pleshakov Date: Fri, 9 Oct 2026 09:36:41 +0300 Subject: [PATCH 16/25] Leave a mistyped action to cobra after misplaced flags (TASK-01) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01XVURGFoSzFiQP2uJD8fLu7 --- ...26-10-09-tasks-flag-order-second-review.md | 91 +++++++++++++++++++ internal/cli/cli_test.go | 5 + internal/cli/flagorder.go | 11 ++- internal/cli/flagorder_test.go | 5 + 4 files changed, 111 insertions(+), 1 deletion(-) create mode 100644 docs/2026-10-09-tasks-flag-order-second-review.md diff --git a/docs/2026-10-09-tasks-flag-order-second-review.md b/docs/2026-10-09-tasks-flag-order-second-review.md new file mode 100644 index 0000000..b61d355 --- /dev/null +++ b/docs/2026-10-09-tasks-flag-order-second-review.md @@ -0,0 +1,91 @@ +# 2026-10-09-tasks-flag-order-second-review.md + +**Status:** Active + +## Context + +A second code review of the branch `command-shape-for-agent-permissions` found +the following, confirmed on the built binary: + +| Invocation | Result | +|---|---| +| `tfscli -p MyProj wit work-items gte 1` | `unknown shorthand flag: 'p' in -p`, where `tfscli wit work-items gte -p MyProj 1` reports `unknown command "gte" ... (did you mean "get"?)` | +| `tfscli --verbose wit wiql query-by-wiql --query "x = 'a' and y = \"b\""` | the corrected command quotes the query as a Go string, `"x = 'a' and y = \"b\""`, which PowerShell does not read back as the same word | +| `tfscli completion bsh` | prints the help of `completion` and exits 0 | + +Decisions already made: + +- When the walk stops at a group, the flags before the path are not checked + against it: a group ignores flags it does not know, and they are usually the + flags of the intended command placed before a mistyped name. The line gets + the "must come first" error, and the corrected command lets cobra report the + mistyped name. `--version` before a group is still reported with the way to + run it. +- The corrected command is shown only when every word can be quoted so that + bash and PowerShell read it alike: a word of safe characters as it is; a word + without a single quote (ASCII or typographic) and without a control character + in single quotes; a word with a single quote in double quotes only when it + contains no `"`, typographic double quote, `$`, backtick, `\`, `!`, or + control character. Otherwise the message says to move the command to the + start and keep every other word in its order, without a command to copy. +- cobra's `completion` group gets the same handling of a leftover argument as + tfscli's own groups. +- A group other than the root defines no flags at all; the root defines only + persistent flags, which are used after the full path. The tree test enforces + exactly that. +- cobra's hidden `__complete` command, with its alias `__completeNoDesc`, is + named in the brief as the one exception to the flag order rule and to the + one-form rule: it runs no command, and the words after it are the command + line to complete. + +--- + +### TASK-01 `group-flags` +**Description:** In `internal/cli/flagorder.go`, check the flags before the +path for unknown or malformed ones only when the command reached has no +subcommands, keeping the `--version` error for a group. Add the mistyped-name +case of the table to `TestFlagBeforeTheCommandPathIsRefused`. +**Definition of done:** `tfscli -p MyProj wit work-items gte 1` gets the "must +come first" error, whose corrected command gets cobra's unknown-command error; +the existing `--version` cases pass; `make check` passes. +**Status:** Done + +### TASK-02 `quoting` +**Description:** Rework `shellJoin` in `internal/cli/flagorder.go` per the +decision above, with the message without a command to copy when a word has no +portable form. Add test cases for a word with `'` and `"`, `'` and `$`, `'` and +`\`, a typographic single quote, and a newline. +**Definition of done:** The corrected command is shown exactly when every word +has a portable form; the new tests pass; `make check` passes. +**Status:** Pending + +### TASK-03 `completion-group` +**Description:** In `internal/cli/cli.go`, share the leftover-argument check of +`newGroupCmd` with cobra's `completion` group, registered in `newRoot`. Cover +`tfscli completion bsh` in `TestUnknownCommandBelowTheRootIsAnError`. +**Definition of done:** `tfscli completion bsh` is an unknown command error +that suggests `bash`; `tfscli completion powershell` still prints the script; +`make check` passes. +**Status:** Pending + +### TASK-04 `tests` +**Description:** Add `-p=X` forms to `oracleWords` and a hand case with +`-p=MyProject` before the action; extract `isCompletionRequest` and use it in +`findPath` and `checkAgainstCobra`; correct the comment on one tree per worker; +make `TestEveryGroupOnlyGroups` reject any flag defined on a group other than +the root. +**Definition of done:** `shorthands` is fully covered; the tests pass; `make +check` passes. +**Status:** Pending + +### TASK-05 `docs` +**Description:** Bring the documents in line with TASK-01 to TASK-04: in +`docs/project-brief.md`, name `__complete` and its alias as the exception; in +`docs/architecture.md`, describe the group case, the quoting rule, the +`completion` group, and the stricter tree test, and drop the claim that every +group is built with `newGroupCmd`; in `flagorder.go`, correct the comments on +group flags and on quoting; in `CHANGELOG.md`, shorten the `Changed` entry and +record the silent no-op and `completion` in `Fixed`. +**Definition of done:** The documents agree with the code and with each other +on these points; `make check` passes. +**Status:** Pending diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 9e3c946..e40beb3 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -593,6 +593,11 @@ func TestUnknownCommandBelowTheRootIsAnError(t *testing.T) { args: []string{"wit", "frobnicate"}, want: "Error [config]: unknown command \"frobnicate\" for \"tfscli wit\" (run \"tfscli wit --help\" for the available commands)\n", }, + { + name: "mistyped action after the corrected flag order", + args: []string{"wit", "work-items", "-p", "MyProject", "gte", "1"}, + want: "Error [config]: unknown command \"gte\" for \"tfscli wit work-items\" (did you mean \"get\"?)\n", + }, { name: "local group", args: []string{"auth", "frobnicate"}, diff --git a/internal/cli/flagorder.go b/internal/cli/flagorder.go index d31c6b4..165c987 100644 --- a/internal/cli/flagorder.go +++ b/internal/cli/flagorder.go @@ -1,6 +1,7 @@ package cli import ( + "errors" "fmt" "slices" "strconv" @@ -35,7 +36,10 @@ func checkFlagOrder(root *cobra.Command, args []string) error { return nil } - if err := unknownFlag(cmd, name, before); err != nil { + // A group ignores the flags it does not know, which before a group are + // usually those of the intended command placed before a mistyped name; + // the corrected command lets cobra report that name instead. + if err := unknownFlag(cmd, name, before); err != nil && (!cmd.HasSubCommands() || errors.Is(err, errVersionFlag)) { return err } return &tfserr.Error{ @@ -157,6 +161,7 @@ func unknownFlag(cmd *cobra.Command, name string, words []string) error { return &tfserr.Error{ Category: tfserr.Config, Message: fmt.Sprintf("--version belongs to tfscli itself, not to %q (run it as: tfscli --version)", name), + Cause: errVersionFlag, } case !found: return &tfserr.Error{Category: tfserr.Config, Message: "unknown flag: --" + flag} @@ -177,6 +182,10 @@ func unknownFlag(cmd *cobra.Command, name string, words []string) error { return nil } +// errVersionFlag is the cause of the error for --version placed before a +// command path. +var errVersionFlag = errors.New("--version before a command path") + // shorthands reads a group of shorthand flags, such as "v" in "-v" or "vpX" // in "-vpX", as pflag reads it, and reports whether the last flag of the group // takes its value from the next word. The help flag, which cobra adds only diff --git a/internal/cli/flagorder_test.go b/internal/cli/flagorder_test.go index f501d43..7ca7e92 100644 --- a/internal/cli/flagorder_test.go +++ b/internal/cli/flagorder_test.go @@ -167,6 +167,11 @@ func TestFlagBeforeTheCommandPathIsRefused(t *testing.T) { args: []string{"--version", "wit"}, want: `Error [config]: --version belongs to tfscli itself, not to "tfscli wit" (run it as: tfscli --version)` + "\n", }, + { + name: "flag of the action before a mistyped action", + args: []string{"-p", "MyProject", "wit", "work-items", "gte", "1"}, + want: refused("tfscli wit work-items", "tfscli wit work-items -p MyProject gte 1"), + }, { name: "unknown flag before the path", args: []string{"--nope", "wit", "work-items", "get", "-p", "MyProject", "12345"}, From ea661ca866c6ee8c7fab5390c2f7f0fa6c302666 Mon Sep 17 00:00:00 2001 From: Dmitry Pleshakov Date: Fri, 9 Oct 2026 09:37:52 +0300 Subject: [PATCH 17/25] Show a corrected command only when both shells read it alike (TASK-02) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01XVURGFoSzFiQP2uJD8fLu7 --- ...26-10-09-tasks-flag-order-second-review.md | 2 +- internal/cli/flagorder.go | 45 +++++++++++++------ internal/cli/flagorder_test.go | 41 ++++++++++++++++- 3 files changed, 72 insertions(+), 16 deletions(-) diff --git a/docs/2026-10-09-tasks-flag-order-second-review.md b/docs/2026-10-09-tasks-flag-order-second-review.md index b61d355..c660856 100644 --- a/docs/2026-10-09-tasks-flag-order-second-review.md +++ b/docs/2026-10-09-tasks-flag-order-second-review.md @@ -57,7 +57,7 @@ portable form. Add test cases for a word with `'` and `"`, `'` and `$`, `'` and `\`, a typographic single quote, and a newline. **Definition of done:** The corrected command is shown exactly when every word has a portable form; the new tests pass; `make check` passes. -**Status:** Pending +**Status:** Done ### TASK-03 `completion-group` **Description:** In `internal/cli/cli.go`, share the leftover-argument check of diff --git a/internal/cli/flagorder.go b/internal/cli/flagorder.go index 165c987..6bdb08c 100644 --- a/internal/cli/flagorder.go +++ b/internal/cli/flagorder.go @@ -4,8 +4,8 @@ import ( "errors" "fmt" "slices" - "strconv" "strings" + "unicode" "github.com/spf13/cobra" @@ -42,10 +42,13 @@ func checkFlagOrder(root *cobra.Command, args []string) error { if err := unknownFlag(cmd, name, before); err != nil && (!cmd.HasSubCommands() || errors.Is(err, errVersionFlag)) { return err } + next := "move it to the start and keep every other word in its order" + if line, ok := shellJoin(append([]string{root.Name()}, corrected...)); ok { + next = "run it as: " + line + } return &tfserr.Error{ Category: tfserr.Config, - Message: fmt.Sprintf("the command %q must come first, before its flags and arguments (run it as: %s)", - name, shellJoin(append([]string{root.Name()}, corrected...))), + Message: fmt.Sprintf("the command %q must come first, before its flags and arguments (%s)", name, next), } } @@ -229,24 +232,40 @@ func lookupFlag(cmd *cobra.Command, name string, short bool) (found, takesValue return true, f.NoOptDefVal == "" } -// shellJoin joins the arguments into a command line. A word with a character -// outside a safe set is put in single quotes, which bash and PowerShell read -// alike; a word that itself contains a single quote is put in double quotes -// instead. -func shellJoin(args []string) string { +// shellJoin joins the arguments into a command line that bash and PowerShell +// read alike, and reports false when a word has no such form. A word with a +// character outside a safe set is put in single quotes, or, when it contains +// a single quote, in double quotes, which the two shells read alike only for a +// word free of the characters either of them interprets there. A control +// character, such as the line break of a multi-line query, is read alike in +// neither. +func shellJoin(args []string) (string, bool) { quoted := make([]string, len(args)) for i, a := range args { switch { + case strings.ContainsFunc(a, unicode.IsControl): + return "", false case a != "" && strings.Trim(a, shellSafe) == "": - case !strings.Contains(a, "'"): + case !strings.ContainsAny(a, singleQuotes): a = "'" + a + "'" + case !strings.ContainsAny(a, doubleQuoted): + a = `"` + a + `"` default: - a = strconv.Quote(a) + return "", false } quoted[i] = a } - return strings.Join(quoted, " ") + return strings.Join(quoted, " "), true } -// shellSafe holds the characters that need no quoting in a shell word. -const shellSafe = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_@%+=:,./-" +// The character sets of shellJoin. shellSafe holds the characters that need +// no quoting in a word, without "@" and "%", with which PowerShell splats a +// variable or stops parsing; singleQuotes the characters that end a single-quoted +// string in bash or PowerShell, which takes typographic quotes for ASCII ones, +// and doubleQuoted the characters that either shell interprets inside double +// quotes. +const ( + shellSafe = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_+=:,./-" + singleQuotes = "'‘’‚‛" + doubleQuoted = "\"“”„$`\\!" +) diff --git a/internal/cli/flagorder_test.go b/internal/cli/flagorder_test.go index 7ca7e92..881ccd6 100644 --- a/internal/cli/flagorder_test.go +++ b/internal/cli/flagorder_test.go @@ -24,6 +24,13 @@ func refused(command, corrected string) string { return `Error [config]: the command "` + command + `" must come first, before its flags and arguments (run it as: ` + corrected + ")\n" } +// unordered is the error for a command line whose command path does not come +// first and that has a word no shell quoting reads alike in bash and +// PowerShell. +func unordered(command string) string { + return `Error [config]: the command "` + command + `" must come first, before its flags and arguments (move it to the start and keep every other word in its order)` + "\n" +} + // A command path that does not open the command line would slip past an agent // permission rule written as a prefix, so it is refused before any request. func TestFlagBeforeTheCommandPathIsRefused(t *testing.T) { @@ -112,6 +119,36 @@ func TestFlagBeforeTheCommandPathIsRefused(t *testing.T) { args: []string{"--verbose", "wit", "wiql", "query-by-wiql", "--query", "it's"}, want: refused("tfscli wit wiql query-by-wiql", `tfscli wit wiql query-by-wiql --verbose --query "it's"`), }, + { + name: "word with a single quote and a backslash", + args: []string{"--verbose", "wit", "wiql", "query-by-wiql", "--query", `UNDER 'Proj\Team'`}, + want: unordered("tfscli wit wiql query-by-wiql"), + }, + { + name: "word with a single and a double quote", + args: []string{"--verbose", "wit", "wiql", "query-by-wiql", "--query", `'a' "b"`}, + want: unordered("tfscli wit wiql query-by-wiql"), + }, + { + name: "word with a single quote and a dollar sign", + args: []string{"--verbose", "wit", "wiql", "query-by-wiql", "--query", "'$x'"}, + want: unordered("tfscli wit wiql query-by-wiql"), + }, + { + name: "word with a typographic single quote", + args: []string{"--verbose", "wit", "wiql", "query-by-wiql", "--query", "it’s"}, + want: refused("tfscli wit wiql query-by-wiql", `tfscli wit wiql query-by-wiql --verbose --query "it’s"`), + }, + { + name: "word with a line break", + args: []string{"--verbose", "wit", "wiql", "query-by-wiql", "--query", "SELECT [System.Id]\nFROM WorkItems"}, + want: unordered("tfscli wit wiql query-by-wiql"), + }, + { + name: "word that PowerShell would splat", + args: []string{"--verbose", "wit", "wiql", "query-by-wiql", "--query", "@x"}, + want: refused("tfscli wit wiql query-by-wiql", "tfscli wit wiql query-by-wiql --verbose --query '@x'"), + }, { name: "flag before help", args: []string{"--verbose", "help", "wit"}, @@ -346,7 +383,7 @@ func TestFlagOrderAgreesWithCobra(t *testing.T) { // checkAgainstCobra checks one command line against the properties of // TestFlagOrderAgreesWithCobra and describes the first one it breaks. func checkAgainstCobra(root *cobra.Command, line []string) string { - shown := shellJoin(append([]string{"tfscli"}, line...)) + shown := fmt.Sprintf("tfscli %q", line) cmd, name, corrected, before := reorder(root, line) err := checkFlagOrder(root, line) switch { @@ -371,7 +408,7 @@ func checkAgainstCobra(root *cobra.Command, line []string) string { if !errors.As(err, &te) { return fmt.Sprintf("%s: refused with an error of no category: %v", shown, err) } - fixed := shellJoin(append([]string{"tfscli"}, corrected...)) + fixed := fmt.Sprintf("tfscli %q", corrected) moved := append(slices.Clip(corrected[:len(strings.Fields(name))-1]), before...) // A group ignores unknown flags, and the flags before a completion // request belong to the root, which is a group too. From 18b651c8be602b04f332826ad6ed1eac698a6ef2 Mon Sep 17 00:00:00 2001 From: Dmitry Pleshakov Date: Fri, 9 Oct 2026 09:38:42 +0300 Subject: [PATCH 18/25] Report an unknown command under completion (TASK-03) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01XVURGFoSzFiQP2uJD8fLu7 --- ...26-10-09-tasks-flag-order-second-review.md | 2 +- internal/cli/cli.go | 58 ++++++++++--------- internal/cli/cli_test.go | 5 ++ 3 files changed, 38 insertions(+), 27 deletions(-) diff --git a/docs/2026-10-09-tasks-flag-order-second-review.md b/docs/2026-10-09-tasks-flag-order-second-review.md index c660856..f7d997c 100644 --- a/docs/2026-10-09-tasks-flag-order-second-review.md +++ b/docs/2026-10-09-tasks-flag-order-second-review.md @@ -66,7 +66,7 @@ has a portable form; the new tests pass; `make check` passes. **Definition of done:** `tfscli completion bsh` is an unknown command error that suggests `bash`; `tfscli completion powershell` still prints the script; `make check` passes. -**Status:** Pending +**Status:** Done ### TASK-04 `tests` **Description:** Add `-p=X` forms to `oracleWords` and a hand case with diff --git a/internal/cli/cli.go b/internal/cli/cli.go index f08cebd..7f41c77 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -105,37 +105,43 @@ func newRoot(b build, stdin prompter, stdout, stderr io.Writer) *cobra.Command { // them here lets checkFlagOrder walk them like any other command. root.InitDefaultHelpCmd() root.InitDefaultCompletionCmd() + completion, _, _ := root.Find([]string{"completion"}) + makeGroup(completion) return root } // newGroupCmd builds a command that only holds subcommands, such as an API -// area or a resource. Cobra rejects an unknown subcommand only on the root; -// below it, a group without RunE prints its help and exits 0, so a mistyped -// resource would pass for success. Here any argument left over after the -// subcommand lookup is reported as an unknown command instead. Unknown flags -// are ignored at this level so that the flags meant for the action do not -// hide the mistyped name. +// area or a resource. func newGroupCmd(use, short string) *cobra.Command { - return &cobra.Command{ - Use: use, - Short: short, - FParseErrWhitelist: cobra.FParseErrWhitelist{UnknownFlags: true}, - // Cobra applies its default distance only when it looks up a - // suggestion itself, which it does on the root alone. - SuggestionsMinimumDistance: 2, - Args: func(cmd *cobra.Command, args []string) error { - if len(args) == 0 { - return nil - } - next := fmt.Sprintf("run %q for the available commands", cmd.CommandPath()+" --help") - if suggestions := cmd.SuggestionsFor(args[0]); len(suggestions) > 0 { - next = fmt.Sprintf("did you mean %q?", suggestions[0]) - } - return fmt.Errorf("unknown command %q for %q (%s)", args[0], cmd.CommandPath(), next) - }, - RunE: func(cmd *cobra.Command, _ []string) error { - return cmd.Help() - }, + cmd := &cobra.Command{Use: use, Short: short} + makeGroup(cmd) + return cmd +} + +// makeGroup makes cmd, a command that only holds subcommands, reject an +// unknown subcommand. Cobra does so only on the root; below it, a group +// without RunE prints its help and exits 0, so a mistyped resource would pass +// for success. Here any argument left over after the subcommand lookup is +// reported as an unknown command instead. Unknown flags are ignored at this +// level so that the flags meant for the action do not hide the mistyped name. +// It also serves cobra's own completion group. +func makeGroup(cmd *cobra.Command) { + cmd.FParseErrWhitelist = cobra.FParseErrWhitelist{UnknownFlags: true} + // Cobra applies its default distance only when it looks up a suggestion + // itself, which it does on the root alone. + cmd.SuggestionsMinimumDistance = 2 + cmd.Args = func(cmd *cobra.Command, args []string) error { + if len(args) == 0 { + return nil + } + next := fmt.Sprintf("run %q for the available commands", cmd.CommandPath()+" --help") + if suggestions := cmd.SuggestionsFor(args[0]); len(suggestions) > 0 { + next = fmt.Sprintf("did you mean %q?", suggestions[0]) + } + return fmt.Errorf("unknown command %q for %q (%s)", args[0], cmd.CommandPath(), next) + } + cmd.RunE = func(cmd *cobra.Command, _ []string) error { + return cmd.Help() } } diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index e40beb3..8103414 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -598,6 +598,11 @@ func TestUnknownCommandBelowTheRootIsAnError(t *testing.T) { args: []string{"wit", "work-items", "-p", "MyProject", "gte", "1"}, want: "Error [config]: unknown command \"gte\" for \"tfscli wit work-items\" (did you mean \"get\"?)\n", }, + { + name: "completion group", + args: []string{"completion", "bsh"}, + want: "Error [config]: unknown command \"bsh\" for \"tfscli completion\" (did you mean \"bash\"?)\n", + }, { name: "local group", args: []string{"auth", "frobnicate"}, From f12db7ba24377bb745574a8d7c69028ade1a729a Mon Sep 17 00:00:00 2001 From: Dmitry Pleshakov Date: Fri, 9 Oct 2026 09:40:48 +0300 Subject: [PATCH 19/25] Cover -p=X, keep group flags off below the root, and share the completion check (TASK-04) The wider oracle alphabet showed that cobra stops at the first flag value it cannot parse, which the check does not read, so the oracle now accepts that outcome instead of expecting a later flag to be reported. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01XVURGFoSzFiQP2uJD8fLu7 --- ...26-10-09-tasks-flag-order-second-review.md | 2 +- internal/cli/flagorder.go | 8 ++++++- internal/cli/flagorder_test.go | 24 ++++++++++++++----- internal/cli/tree_test.go | 10 +++++--- 4 files changed, 33 insertions(+), 11 deletions(-) diff --git a/docs/2026-10-09-tasks-flag-order-second-review.md b/docs/2026-10-09-tasks-flag-order-second-review.md index f7d997c..3db5eed 100644 --- a/docs/2026-10-09-tasks-flag-order-second-review.md +++ b/docs/2026-10-09-tasks-flag-order-second-review.md @@ -76,7 +76,7 @@ make `TestEveryGroupOnlyGroups` reject any flag defined on a group other than the root. **Definition of done:** `shorthands` is fully covered; the tests pass; `make check` passes. -**Status:** Pending +**Status:** Done ### TASK-05 `docs` **Description:** Bring the documents in line with TASK-01 to TASK-04: in diff --git a/internal/cli/flagorder.go b/internal/cli/flagorder.go index 6bdb08c..07cee4c 100644 --- a/internal/cli/flagorder.go +++ b/internal/cli/flagorder.go @@ -111,7 +111,7 @@ func findPath(root *cobra.Command, line []string) (cmd *cobra.Command, name stri cmd = root words := []string{root.Name()} for i, arg := range line { - if cmd == root && (arg == cobra.ShellCompRequestCmd || arg == cobra.ShellCompNoDescRequestCmd) { + if cmd == root && isCompletionRequest(arg) { words = append(words, arg) path = append(path, i) break @@ -125,6 +125,12 @@ func findPath(root *cobra.Command, line []string) (cmd *cobra.Command, name stri return cmd, strings.Join(words, " "), path } +// isCompletionRequest reports whether word names cobra's hidden command for +// shell completion requests. +func isCompletionRequest(word string) bool { + return word == cobra.ShellCompRequestCmd || word == cobra.ShellCompNoDescRequestCmd +} + // subcommand returns the child of cmd named name, or nil. func subcommand(cmd *cobra.Command, name string) *cobra.Command { for _, c := range cmd.Commands() { diff --git a/internal/cli/flagorder_test.go b/internal/cli/flagorder_test.go index 881ccd6..8dc7b34 100644 --- a/internal/cli/flagorder_test.go +++ b/internal/cli/flagorder_test.go @@ -64,6 +64,11 @@ func TestFlagBeforeTheCommandPathIsRefused(t *testing.T) { args: []string{"wit", "work-items", "-pMyProject", "get", "12345"}, want: refused("tfscli wit work-items get", "tfscli wit work-items get -pMyProject 12345"), }, + { + name: "shorthand with a value after an equals sign", + args: []string{"wit", "work-items", "-p=MyProject", "get", "12345"}, + want: refused("tfscli wit work-items get", "tfscli wit work-items get -p=MyProject 12345"), + }, { name: "long help flag before the path", args: []string{"--help", "wit", "work-items", "get"}, @@ -350,7 +355,8 @@ func TestFlagOrderAgreesWithCobra(t *testing.T) { var wg sync.WaitGroup for range runtime.GOMAXPROCS(0) { wg.Go(func() { - // The check only reads the tree, so one tree serves a worker. + // The check merges persistent flags into the tree it walks, so + // each worker builds its own. root := newRoot(testBuild, noTerminal{}, io.Discard, io.Discard) for line := range next { if msg := checkAgainstCobra(root, line); msg != "" { @@ -392,7 +398,7 @@ func checkAgainstCobra(root *cobra.Command, line []string) string { case corrected == nil: // Cobra serves a completion request by construction; executing it // would only print cobra's completion diagnostics to the real stderr. - if len(line) > 0 && (line[0] == cobra.ShellCompRequestCmd || line[0] == cobra.ShellCompNoDescRequestCmd) { + if len(line) > 0 && isCompletionRequest(line[0]) { return "" } got := selected(line) @@ -418,7 +424,10 @@ func checkAgainstCobra(root *cobra.Command, line []string) string { if !leaf { return "" } + // Cobra stops at the first flag value it cannot parse, which the + // check does not read, so a later flag goes unreported. switch got := flagError(moved); { + case strings.HasPrefix(got, invalidValue): case got == "": return fmt.Sprintf("%s: refused with %q, but cobra accepts the flags before the path", shown, te.Message) case got != te.Message && !strings.HasPrefix(te.Message, "--version"): @@ -438,12 +447,15 @@ func checkAgainstCobra(root *cobra.Command, line []string) string { if !leaf { return "" } - if got := flagError(moved); got != "" { + if got := flagError(moved); got != "" && !strings.HasPrefix(got, invalidValue) { return fmt.Sprintf("%s: corrected to %s, though cobra rejects a flag before the path: %s", shown, fixed, got) } return "" } +// invalidValue opens cobra's message for a flag value it cannot parse. +const invalidValue = "invalid argument " + // selected returns the path, as typed, of the command cobra selects for args, // whether it then runs the command, prints its help, or rejects its flags or // arguments. @@ -458,7 +470,7 @@ func selected(args []string) string { } // flagError returns the message with which cobra rejects a flag of args as -// unknown or malformed, or "" if it rejects none. +// unknown or malformed or its value as invalid, or "" if it rejects none. func flagError(args []string) string { root := stubbedRoot() root.SetArgs(args) @@ -466,7 +478,7 @@ func flagError(args []string) string { if err == nil { return "" } - for _, kind := range []string{"unknown flag: ", "unknown shorthand flag: ", "bad flag syntax: "} { + for _, kind := range []string{"unknown flag: ", "unknown shorthand flag: ", "bad flag syntax: ", invalidValue} { if strings.HasPrefix(err.Error(), kind) { return err.Error() } @@ -513,7 +525,7 @@ func oracleWords() []string { if short != "" { words = append(words, "-"+short) if value { - words = append(words, "-"+short+"X") + words = append(words, "-"+short+"X", "-"+short+"=X") } } } diff --git a/internal/cli/tree_test.go b/internal/cli/tree_test.go index 5bdc116..c7289e8 100644 --- a/internal/cli/tree_test.go +++ b/internal/cli/tree_test.go @@ -33,8 +33,8 @@ func TestEveryCommandHasOneForm(t *testing.T) { // checkFlagOrder takes every word naming a subcommand as a step of the path, // which holds only while a command with subcommands just groups them: a // group's own argument or flag value could otherwise name a subcommand and be -// taken for one. Persistent flags are allowed, since they are used after the -// full path. +// taken for one. Only the root may define flags, and only persistent ones, +// which are used after the full path. func TestEveryGroupOnlyGroups(t *testing.T) { var walk func(cmd *cobra.Command) walk = func(cmd *cobra.Command) { @@ -46,7 +46,11 @@ func TestEveryGroupOnlyGroups(t *testing.T) { if (cmd.HasParent() || cmd.Args != nil) && cmd.ValidateArgs([]string{"x"}) == nil { t.Errorf("%q accepts an argument; a command with subcommands takes none", cmd.CommandPath()) } - if own := cmd.LocalNonPersistentFlags(); own.HasFlags() { + own := cmd.LocalNonPersistentFlags() + if cmd.HasParent() { + own = cmd.LocalFlags() + } + if own.HasFlags() { t.Errorf("%q defines flags of its own; a command with subcommands has none:\n%s", cmd.CommandPath(), own.FlagUsages()) } for _, child := range cmd.Commands() { From 2c7d26f6fc5e69fa8e8a5aff0f69d016cc0b9fa9 Mon Sep 17 00:00:00 2001 From: Dmitry Pleshakov Date: Fri, 9 Oct 2026 09:42:39 +0300 Subject: [PATCH 20/25] Align the docs with the second branch review (TASK-05) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01XVURGFoSzFiQP2uJD8fLu7 --- CHANGELOG.md | 5 +++-- docs/architecture.md | 2 +- .../2026-10-09-tasks-flag-order-second-review.md | 4 ++-- docs/project-brief.md | 4 ++-- internal/cli/flagorder.go | 14 ++++++++------ skills/tfscli/SKILL.md | 5 +++-- 6 files changed, 19 insertions(+), 15 deletions(-) rename docs/{ => archive}/2026-10-09-tasks-flag-order-second-review.md (99%) diff --git a/CHANGELOG.md b/CHANGELOG.md index c08fafc..357b80d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,10 +8,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Changed -- A flag placed before the end of the command path, such as `--verbose` in `tfscli --verbose wit work-items get 12345` or `--help` in `tfscli --help wit work-items get`, is now refused with a `config` error that shows the command with the flag moved after it, where it used to be accepted, to fail with a misleading "unknown command", or, for a flag of an action placed before the action, as in `tfscli wit wiql --time-precision query-by-wiql`, to print the help of the group and exit successfully without running the query; flags now always follow the command, so that agent permission rules can match the command by its name. +- A flag placed before the end of the command path, such as `--verbose` in `tfscli --verbose wit work-items get 12345` or `--help` in `tfscli --help wit work-items get`, is now refused with a `config` error that shows how to run the command with its flags after it, so that agent permission rules can match the command by its name. ### Fixed -- An unknown command under `tfscli auth`, such as `tfscli auth logn`, is now reported as an unknown command, where tfscli used to print the help of `auth` and exit successfully. +- An unknown command under `tfscli auth` or `tfscli completion`, such as `tfscli auth logn`, is now reported as an unknown command, where tfscli used to print the help of the group and exit successfully. +- A flag of an action placed before the action, as in `tfscli wit wiql --time-precision query-by-wiql --query "..."`, no longer makes tfscli print the help of the group and exit successfully without running the query; the command line is refused with the corrected command instead. ## [0.1.1] - 2026-10-06 diff --git a/docs/architecture.md b/docs/architecture.md index 38a78ba..fdd9e1a 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -195,7 +195,7 @@ This section describes the conceptual module structure, their responsibilities, ### Modules and responsibilities -- **cli** — cobra commands, nested as area, resource, and action after the REST API reference (`wit` → `work-items` → `get`); persistent flags (`--verbose`, config overrides); orchestrates the chain config → apiclient → domain → printer → exit. Owns the markdown printer functions (one per resource and shape, such as a single work item and several) plus small per-kind scalar formatters for identity and datetime values used by those printers. No `Renderer` interface in v1 — printers are plain functions. When `--json` is added later, the interface and a second renderer can be introduced here without touching the domain layer. Enforces the command line shape that "The command line can be governed by permission rules" in `project-brief.md` requires. Cobra accepts flags at any position, so `run` checks the arguments before executing the tree: the path of the command must occupy exactly the first words of the command line, and otherwise the command fails with a `config` error that shows the corrected command — the path followed by every other word in its original order, quoted for the shell in single quotes, which bash and PowerShell read alike, or in double quotes for a word that itself contains a single quote. The command is the one reached from the root by taking, in order, every word that names a subcommand of the command reached so far; flag values are not read, and `--` does not stop the search, since pflag takes it for the value of a flag placed before it. When moving the path to the front turns a word that preceded one of its steps into a further step, as `login` in `tfscli - login auth`, the move is repeated, so that the corrected command is itself accepted. This depends on the shape of the tree: a group takes no arguments and defines no flags of its own, and the flags cobra adds itself, `--help` and the root's `--version`, take no value, so in a valid command line no word naming a subcommand can follow the path, and the rule reaches at least as deep as cobra's `Find`, which descends only through such words. A rule that read flag values to find the path was rejected: it is a second parser of the command line, and each of its divergences from pflag let a command run with its path not at the start. `newRoot` registers cobra's `help` and `completion` commands up front so that they are found like any other command. Cobra's hidden `__complete` command, which the shell completion scripts run, is added only when cobra executes the tree, so the check takes it for a subcommand of the root that ends the path: the words after it are the command line to complete. A flag before the path that the command does not define, or that is malformed, is reported as such, in pflag's wording, rather than moved into a corrected command that would fail the same way; the words before the path are read as pflag reads them, so a flag's value and the words after `--` are not taken for flags. `--version`, defined on the root alone, is refused before a path with a message that names `tfscli --version`. The check is explicit rather than a root `PersistentPreRunE`, because cobra runs only the nearest such hook and a child command defining its own would silently drop it. A test walks the whole command tree and fails on any command alias, on cobra's prefix or case-insensitive matching, and on a command with subcommands that accepts an argument or defines a flag of its own, so that each command keeps exactly one form and the check keeps the shape it relies on. Another test takes cobra as the oracle: over every command line of up to three words built from the names and flags of the tree and the words pflag treats specially, and a fixed-seed sample of longer lines, it requires that a line the check accepts makes cobra select a command whose path opens the line, that the corrected command proposed for a refused line is accepted, makes cobra select the command the message names, and leaves cobra no flag before the path to reject, and that a line refused for a flag before the path is rejected by cobra with the same message once the path is moved to the front. The tests turn off cobra's check, on Windows, of whether the process was started from Explorer: it lists every process of the system before each execution, which would make this test take minutes. Every group, local ones included, is built with `newGroupCmd`. +- **cli** — cobra commands, nested as area, resource, and action after the REST API reference (`wit` → `work-items` → `get`); persistent flags (`--verbose`, config overrides); orchestrates the chain config → apiclient → domain → printer → exit. Owns the markdown printer functions (one per resource and shape, such as a single work item and several) plus small per-kind scalar formatters for identity and datetime values used by those printers. No `Renderer` interface in v1 — printers are plain functions. When `--json` is added later, the interface and a second renderer can be introduced here without touching the domain layer. Enforces the command line shape that "The command line can be governed by permission rules" in `project-brief.md` requires. Cobra accepts flags at any position, so `run` checks the arguments before executing the tree: the path of the command must occupy exactly the first words of the command line, and otherwise the command fails with a `config` error that names the command and shows the corrected command — the path followed by every other word in its original order — when every word has a form that bash and PowerShell read alike. A word outside a safe set of characters is put in single quotes; `@` and `%` are outside it, since PowerShell splats a variable after `@` and stops parsing at `--%`. A word with a single quote, ASCII or typographic (PowerShell takes both for a quote), is put in double quotes, provided it holds none of the characters either shell interprets there: `"`, a typographic double quote, `$`, a backtick, `\`, or `!`. A word with a control character, such as a multi-line query, or with a single quote and one of those characters has no such form; the message then says to move the command to the start and keep every other word in its order. The command is the one reached from the root by taking, in order, every word that names a subcommand of the command reached so far; flag values are not read, and `--` does not stop the search, since pflag takes it for the value of a flag placed before it. When moving the path to the front turns a word that preceded one of its steps into a further step, as `login` in `tfscli - login auth`, the move is repeated, so that the corrected command is itself accepted. This depends on the shape of the tree: a group takes no arguments, a group below the root defines no flags, the root defines only persistent ones, and the flags cobra adds itself, `--help` and the root's `--version`, take no value, so a word naming a subcommand can follow the path only as the value of a persistent flag given to a group, which does nothing but print its help, and the rule reaches at least as deep as cobra's `Find`, which descends only through such words. A rule that read flag values to find the path was rejected: it is a second parser of the command line, and each of its divergences from pflag let a command run with its path not at the start. `newRoot` registers cobra's `help` and `completion` commands up front so that they are found like any other command. Cobra's hidden `__complete` command, also accepted as `__completeNoDesc`, which the shell completion scripts run, is added only when cobra executes the tree, so the check takes it for a subcommand of the root that ends the path: the words after it are the command line to complete. A flag before the path that the command does not define, or that is malformed, is reported as such, in pflag's wording, rather than moved into a corrected command that would fail the same way; the words before the path are read as pflag reads them, so a flag's value and the words after `--` are not taken for flags. This applies when the command is an action: a group ignores the flags it does not know, and before a group they are usually the flags of the intended action placed before a mistyped name, so the line gets the corrected command, on which cobra reports the mistyped name. `--version`, defined on the root alone, is refused before a path with a message that names `tfscli --version`. The check is explicit rather than a root `PersistentPreRunE`, because cobra runs only the nearest such hook and a child command defining its own would silently drop it. A test walks the whole command tree and fails on any command alias (cobra adds `__complete` only when it executes the tree, so its alias lies outside the test), on cobra's prefix or case-insensitive matching, and on a command with subcommands that accepts an argument or defines a flag other than a persistent flag of the root, so that each command keeps exactly one form and the check keeps the shape it relies on. Another test takes cobra as the oracle: over every command line of up to three words built from the names and flags of the tree and the words pflag treats specially, and a fixed-seed sample of longer lines, it requires that a line the check accepts makes cobra select a command whose path opens the line, that the corrected command proposed for a refused line is accepted, makes cobra select the command the message names, and leaves cobra no flag before the path to reject, and that a line refused for a flag before the path is rejected by cobra with the same message once the path is moved to the front, unless cobra first stops at a flag value it cannot parse, which the check does not read. The tests turn off cobra's check, on Windows, of whether the process was started from Explorer: it lists every process of the system before each execution, which would make this test take minutes. Every group of tfscli, local ones included, is built with `newGroupCmd`, and `newRoot` gives cobra's `completion` group the same handling through `makeGroup`, so that an unknown command below any group is an error rather than its help. - **config** — resolves the credential (`URL`, `Collection`, and `PAT`; a PAT is issued for one collection) from `TFSCLI_AUTH` or `$XDG_DATA_HOME/tfscli/auth.json`, never from a mix of sources; loads the optional `$XDG_CONFIG_HOME/tfscli/config.json`, applies environment overrides (`TFSCLI_*`), applies cobra flag overrides; validates that all required fields are set for the command being run. Writes `auth.json` for `auth login` (mode `0600`, directory `0700`). Holds `URL`, `Collection`, `PAT`, `Project`, `APIVersion`, `InsecureSkipVerify`, `CABundle`. Never logs PAT. - **apiclient** — wraps `net/http`. Builds URLs from `Config.URL + Collection + path` and adds `api-version`. A configured `APIVersion` is sent as it is. Otherwise the version is negotiated per request, as the official SDKs do: `OPTIONS {URL}/{Collection}/_apis` lists every resource location with its versions, the caller names its location by id, and the request carries the location's released version, or `{maxVersion}-preview.{resourceVersion}` when it has none — the version the server chooses for a request without one. The response is not cached. A 401 or a transport failure on `OPTIONS` fails the request; any other failure sends it without a version and notes why through `Logger.Info`, and a refusal of that request for the missing version gets the settings to use in its message. A call without a location, the login check, neither negotiates nor sends a version. Sets `Authorization: Basic base64(":")` on every request. Configures TLS using `CABundle` (appended to system root pool) and `InsecureSkipVerify`. Hooks the logger via a `RoundTripper` — that transport is the single call site of `LogRequest`, so every outgoing request is logged exactly once and no other module logs HTTP traffic. Classifies HTTP outcomes into the stable error categories defined in the brief (`auth`, `not_found`, `forbidden`, `server`, `config`, `network`). - **workitem** — domain logic for Work Items: Get Work Item, Work Items - List, and Get Work Items Batch. Owns the `WorkItem`/`Field` types, the `BatchRequest`/`Batch` types for reading several work items, and the allowlist of fields known to contain HTML (`System.Description`, `Microsoft.VSTS.TCM.ReproSteps`, `Microsoft.VSTS.TCM.SystemInfo`, `Microsoft.VSTS.Common.AcceptanceCriteria`, …). After unmarshalling, tags each field with its `FieldKind`. For several work items, keeps the server's order and reports the requested ids the server did not return. Returns raw values — does not perform markdown conversion. diff --git a/docs/2026-10-09-tasks-flag-order-second-review.md b/docs/archive/2026-10-09-tasks-flag-order-second-review.md similarity index 99% rename from docs/2026-10-09-tasks-flag-order-second-review.md rename to docs/archive/2026-10-09-tasks-flag-order-second-review.md index 3db5eed..181ee23 100644 --- a/docs/2026-10-09-tasks-flag-order-second-review.md +++ b/docs/archive/2026-10-09-tasks-flag-order-second-review.md @@ -1,6 +1,6 @@ # 2026-10-09-tasks-flag-order-second-review.md -**Status:** Active +**Status:** Archived ## Context @@ -88,4 +88,4 @@ group flags and on quoting; in `CHANGELOG.md`, shorten the `Changed` entry and record the silent no-op and `completion` in `Fixed`. **Definition of done:** The documents agree with the code and with each other on these points; `make check` passes. -**Status:** Pending +**Status:** Done diff --git a/docs/project-brief.md b/docs/project-brief.md index d15703c..67321dd 100644 --- a/docs/project-brief.md +++ b/docs/project-brief.md @@ -61,8 +61,8 @@ An AI agent can call `tfscli`, read work item content, and use it meaningfully i - A path parameter that identifies the resource is a positional argument: `wit work-items get 12345` for `_apis/wit/workitems/{id}`. The project, the team, and the collection, which scope the request rather than identify the resource, are not: the collection comes from the credential, the project from `-p` or its defaults, and the team from `--team`. - A query or body parameter is a flag with the API name in kebab-case and without a leading `$`: `ids` is `--ids`, `errorPolicy` is `--error-policy`, `$expand` is `--expand`. A flag that is not given is not sent, so the server's default applies. - **The command line can be governed by permission rules.** AI agent hosts, such as Claude Code and opencode, decide whether an agent may run a command by matching its text against allow, ask, and deny rules. tfscli keeps that text predictable: - - The command path opens the command line, and flags follow its last word: `tfscli wit work-items get --verbose 12345`, not `tfscli --verbose wit work-items get 12345`. A flag placed earlier is refused with a `config` error that shows the corrected command. The rule has no exceptions: `--help` follows the path like any other flag, and the built-in `help` and `completion` commands are commands like any other. Only an invocation without a command path, such as `tfscli --help` or `tfscli --version`, has no path for a flag to precede; `--version` belongs to the root alone. - - Each command has exactly one form: no command aliases, no abbreviated command or flag names, no prefix or case-insensitive matching of command names. + - The command path opens the command line, and flags follow its last word: `tfscli wit work-items get --verbose 12345`, not `tfscli --verbose wit work-items get 12345`. A flag placed earlier is refused with a `config` error that shows the corrected command. `--help` follows the path like any other flag, and the built-in `help` and `completion` commands are commands like any other. The one exception is cobra's hidden `__complete` command, which the shell completion scripts run with the words typed so far after it: those words are the command line to complete, not a command that runs, so they may hold flags before a path. Only an invocation without a command path, such as `tfscli --help` or `tfscli --version`, has no path for a flag to precede; `--version` belongs to the root alone. + - Each command has exactly one form: no command aliases, no abbreviated command or flag names, no prefix or case-insensitive matching of command names. The hidden `__complete` command, which cobra also accepts as `__completeNoDesc`, is the one command with two names; it runs no command of tfscli. - A command that has subcommands — an area, a resource, or a local group such as `auth` — only groups them: it takes no arguments, defines no flags of its own, and does nothing but print its help. Only a command without subcommands runs. A group that also acts on its own, as `git stash` and `git remote` do, is excluded: a rule written for the group would also cover every command below it, and the command path could no longer be told from the group's own arguments. - The names of local commands (`auth`, `licenses`, `help`, `completion`) never coincide with an area of the REST API reference. - Whether a command changes server state is a property of the command, never of its flags: a reading command does not gain a side effect through a flag. There is no generic pass-through command that sends an arbitrary request; should one be added, it falls under the gate flags described in "Non-Goals". diff --git a/internal/cli/flagorder.go b/internal/cli/flagorder.go index 07cee4c..88d282a 100644 --- a/internal/cli/flagorder.go +++ b/internal/cli/flagorder.go @@ -20,12 +20,14 @@ import ( // The command is the one reached from the root by taking, in order, every word // that names a subcommand of the command reached so far. The walk does not // stop at "--", since pflag takes it for the value of a flag placed before it. -// No knowledge of flag values is needed: a group takes no arguments and -// defines no flags of its own (TestEveryGroupOnlyGroups), and the flags cobra -// adds itself, --help and the root's --version, take no value, so in a valid -// command line no word naming a subcommand can follow the path. This reaches -// at least as deep as cobra's Find, which descends only through such words, -// so cobra never runs a command whose path is not at the start. +// No knowledge of flag values is needed: a group takes no arguments, a group +// below the root defines no flags, the root defines only persistent ones +// (TestEveryGroupOnlyGroups), and the flags cobra adds itself, --help and the +// root's --version, take no value. So a word naming a subcommand can follow +// the path only as the value of a persistent flag given to a group, which +// does nothing but print its help. The walk reaches at least as deep as +// cobra's Find, which descends only through such words, so cobra never runs a +// command whose path is not at the start. // // Cobra's hidden command for shell completion requests, which it adds to the // root only when it executes the tree, is taken as a subcommand of the root diff --git a/skills/tfscli/SKILL.md b/skills/tfscli/SKILL.md index 96e41c4..29b3861 100644 --- a/skills/tfscli/SKILL.md +++ b/skills/tfscli/SKILL.md @@ -134,8 +134,9 @@ so `-p` can carry everything a call needs beyond the credential. Write the full command name first and every flag after it: `tfscli wit work-items get --verbose -p `. A flag before the last word of the command, as in `tfscli --verbose wit work-items get ...`, is refused -with a `config` error that shows the corrected command, and so is `--help` in -that place: ask for help as `tfscli wit work-items get --help`. The user's +with a `config` error that shows the corrected command (or, for a word no +quoting reads alike in bash and PowerShell, says to move the command to the +start), and so is `--help` in that place: ask for help as `tfscli wit work-items get --help`. The user's permission rules match the command by its name, so keep that order rather than relying on the error. From 354e28f8d69f42197d4468243a6efabd181d32b0 Mon Sep 17 00:00:00 2001 From: Dmitry Pleshakov Date: Sat, 10 Oct 2026 09:07:58 +0300 Subject: [PATCH 21/25] Generalize the permission rules in the README The ask rule on --allow-changes matched any command carrying the string, so it also prompted for unrelated commands such as a search or a commit message that mentions the flag; it now starts with tfscli like the other rules. The section states the rules and the precedence they need independently of an agent, and keeps the Claude Code and opencode settings as examples whose format belongs to the agent. The section is unwrapped to match the rest of the file. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01HmjswYSvuZ7yb3erKTWJH3 --- README.md | 76 +++++++++++++++++++++---------------------------------- 1 file changed, 29 insertions(+), 47 deletions(-) diff --git a/README.md b/README.md index 84d8f09..c2e26b4 100644 --- a/README.md +++ b/README.md @@ -28,7 +28,7 @@ Not present in v1: write operations (out of scope) and JSON output (`--json`, pl 5. Copy the `skills/tfscli` directory from the archive into the skills directory of your AI agent. -Details for each step: [Installation](#installation) covers the archive names, checksums, and building from source; [Authentication](#authentication) covers the token scope, Git Bash on Windows, and CI; [Use with an AI agent](#use-with-an-ai-agent) lists the skills directories of the supported agents, gives the copy commands, and describes an agent without skill support. Further settings, such as a default project, are described under [Configuration](#configuration). +Details for each step: [Installation](#installation) covers the archive names, checksums, and building from source; [Authentication](#authentication) covers the token scope, Git Bash on Windows, and CI; [Use with an AI agent](#use-with-an-ai-agent) lists the skills directories of Claude Code and opencode, gives the copy commands, and describes an agent without skill support. Further settings, such as a default project, are described under [Configuration](#configuration). ## Installation @@ -115,15 +115,9 @@ PAT is the only authentication method; SSPI and NTLM are out of scope. The token ## Use with an AI agent -tfscli is built for AI coding agents first, and every archive carries an agent -skill beside the binary: `skills/tfscli/SKILL.md`, in the Agent Skills format. -It tells an agent when the tool applies, how the output is shaped, what each -error category calls for, and where the boundaries are — none of which `--help` -conveys. +tfscli is built for AI coding agents first, and every archive carries an agent skill beside the binary: `skills/tfscli/SKILL.md`, in the Agent Skills format. It tells an agent when the tool applies, how the output is shaped, what each error category calls for, and where the boundaries are — none of which `--help` conveys. -Install it by copying the directory. Create the destination first: copying -into a path that does not exist yet leaves `SKILL.md` one level too high, -where no agent looks for it. +Install it by copying the directory. Create the destination first: copying into a path that does not exist yet leaves `SKILL.md` one level too high, where no agent looks for it. ```sh mkdir -p ~/.claude/skills && cp -r skills/tfscli ~/.claude/skills/ @@ -134,41 +128,39 @@ New-Item -ItemType Directory -Force $HOME\.claude\skills | Out-Null Copy-Item -Recurse skills\tfscli $HOME\.claude\skills\ ``` -That one location serves both agents: opencode has first-party skill support and -searches `~/.claude/skills/` among its own paths. Other locations work as well, -if the skill should be scoped to a single project or kept out of the Claude -directory: +That one location serves Claude Code and opencode: opencode has first-party skill support and searches `~/.claude/skills/` among its own paths. Other locations work as well, if the skill should be scoped to a single project or kept out of the Claude directory: | Agent | Project | Global | |---|---|---| | Claude Code | `.claude/skills/tfscli/` | `~/.claude/skills/tfscli/` | | opencode | `.opencode/skills/tfscli/`, `.claude/skills/tfscli/`, `.agents/skills/tfscli/` | `~/.config/opencode/skills/tfscli/`, `~/.claude/skills/tfscli/`, `~/.agents/skills/tfscli/` | -In opencode, loading a skill is subject to `permission.skill` in -`opencode.json` — `allow`, `ask`, or `deny` — and the mechanism can be switched -off altogether with `tools.skill: false`. If the skill is never offered, check -those settings first. +In opencode, loading a skill is subject to `permission.skill` in `opencode.json` — `allow`, `ask`, or `deny` — and the mechanism can be switched off altogether with `tools.skill: false`. If the skill is never offered, check those settings first. -An agent without skill support can be pointed at the same file directly: it is -plain markdown under a short YAML header. +An agent without skill support can be pointed at the same file directly: it is plain markdown under a short YAML header. ### Permission rules -Both agents decide whether a shell command may run by matching its text against -rules. Every tfscli command starts with its full name, such as -`tfscli wit work-items get` or `tfscli licenses`, followed by flags and -arguments, and each command has exactly one name, so a rule written as a prefix -covers the command whatever its flags. The current commands only read; -`auth login` is interactive and is not for an agent to run. +An agent usually decides whether a shell command may run by matching its text against allow, ask, and deny rules. Every tfscli command starts with its full name, such as `tfscli wit work-items get` or `tfscli licenses`, followed by flags and arguments, and each command has exactly one name, so a rule written as a prefix covers the command whatever its flags. The current commands only read; `auth login` is interactive and is not for an agent to run. -Commands that change data are not part of tfscli yet. When they are added, -each will require the flag `--allow-changes` and will fall under `tfscli wit *` -like the reading commands, so the examples below already ask before a command -that carries that flag as it is usually written. +Commands that change data are not part of tfscli yet. When they are added, each will require the flag `--allow-changes` and will fall under `tfscli wit *` like the reading commands. -Claude Code, in `.claude/settings.json` or `~/.claude/settings.json`; the -`PowerShell` rules apply where the agent runs commands through PowerShell. An -`ask` rule takes precedence over an `allow` rule: +The following rules let an agent read freely, ask before a command that changes data, and keep the agent away from the credential. `*` stands for any text, spaces included: + +| Command | Decision | +|---|---| +| `tfscli wit *` | allow | +| `tfscli licenses` | allow | +| `tfscli *--allow-changes*` | ask | +| `tfscli auth *` | deny | + +A command that changes data matches both `tfscli wit *` and `tfscli *--allow-changes*`, so the rules work only if the ask rule takes precedence over the allow rule. Agents resolve such a conflict differently: some rank the decisions, others apply the last matching rule. + +A rule matches the command as the agent types it. A call through `tfscli.exe` or a full path matches none of these rules and gets the agent's default for unmatched commands, which may be to allow it. Likewise, a flag written with quotes in the middle, such as `--allow-chan''ges`, reaches tfscli as `--allow-changes` but may not match the rule. The rules therefore govern the usual form of a call and guard against an agent's mistake; they are not a security boundary against an agent that sets out to get around them. + +The examples below write the same rules for two agents. Each agent owns its settings format, which may change; its own documentation takes precedence over these examples. + +Claude Code, in `.claude/settings.json` or `~/.claude/settings.json`. Claude Code matches commands of its Bash and PowerShell tools by separate rules, so each rule appears in both forms. A `deny` rule takes precedence over an `ask` rule, and an `ask` rule over an `allow` rule, whatever their order. A command that matches no rule is asked about in the default mode: ```json { @@ -180,8 +172,8 @@ Claude Code, in `.claude/settings.json` or `~/.claude/settings.json`; the "PowerShell(tfscli licenses)" ], "ask": [ - "Bash(*--allow-changes*)", - "PowerShell(*--allow-changes*)" + "Bash(tfscli *--allow-changes*)", + "PowerShell(tfscli *--allow-changes*)" ], "deny": [ "Bash(tfscli auth *)", @@ -191,8 +183,7 @@ Claude Code, in `.claude/settings.json` or `~/.claude/settings.json`; the } ``` -opencode, in `opencode.json`. The last matching rule wins, so place these after -any catch-all `"*"` rule, in this order: +opencode, in `opencode.json`. The last matching rule wins, so place these after any catch-all `"*"` rule, in this order. A command that matches no rule is allowed unless a catch-all `"*": "ask"` is set: ```json { @@ -200,22 +191,13 @@ any catch-all `"*"` rule, in this order: "bash": { "tfscli wit *": "allow", "tfscli licenses": "allow", - "*--allow-changes*": "ask", + "tfscli *--allow-changes*": "ask", "tfscli auth *": "deny" } } } ``` -A rule matches the command as the agent types it. A call through `tfscli.exe` -or a full path matches none of the `tfscli` rules and gets the agent's default -for unmatched commands: Claude Code asks in its default mode, while opencode -allows it unless a catch-all `"*": "ask"` is set. Likewise, a flag written -with quotes in the middle, such as `--allow-chan''ges`, reaches tfscli as -`--allow-changes` but may not match the rule. The rules therefore govern the -usual form of a call and guard against an agent's mistake; they are not a -security boundary against an agent that sets out to get around them. - ## Configuration Every setting apart from the credential: @@ -277,7 +259,7 @@ tfscli [flags] [arguments] Commands are named after the REST API reference: the area and the resource are the segments of the operation's page path, so Get Work Item, documented under `.../wit/work-items/get-work-item`, is `wit work-items get`. `auth login` and `licenses` are local to tfscli and have no counterpart in the API. -Flags follow the command: `tfscli wit work-items get --verbose -p MyProject 12345`, never `tfscli --verbose wit work-items get ...`. A flag placed before the last word of the command is refused with a `config` error that shows the command with the flag moved, and nothing is sent to the server; a flag that the command does not define is reported as unknown instead. `--help` is no exception: `tfscli wit work-items get --help`, not `tfscli --help wit work-items get`. `--version` belongs to `tfscli` alone, and placed before a command it is refused with a message that names `tfscli --version`. The rule keeps the command at the start of the command line, where agent permission rules look for it (see "Permission rules"). +Flags follow the command: `tfscli wit work-items get --verbose -p MyProject 12345`, never `tfscli --verbose wit work-items get ...`. A flag placed before the last word of the command is refused with a `config` error that shows the command with the flag moved, and nothing is sent to the server; a flag that the command does not define is reported as unknown instead. `--help` is no exception: `tfscli wit work-items get --help`, not `tfscli --help wit work-items get`. `--version` belongs to `tfscli` alone, and placed before a command it is refused with a message that names `tfscli --version`. The rule keeps the command at the start of the command line, where agent permission rules look for it (see [Permission rules](#permission-rules)). `tfscli --version` prints the version and the commit it was built from; `tfscli --help`, and `--help` on any command, lists the flags. `tfscli licenses` prints the components built into the binary — tfscli itself, the Go standard library, and the third-party modules — followed by the license text of each. From efe027c177bb869c37315d531606e0d816fa0862 Mon Sep 17 00:00:00 2001 From: Dmitry Pleshakov Date: Sat, 10 Oct 2026 09:10:58 +0300 Subject: [PATCH 22/25] Shorten the constraints repeated in CONTRIBUTING The list repeats the brief to catch a violation without opening it, so it names each rule and leaves the detail to the brief. The command line rule keeps every part but drops the parenthetical expansions, which tests in internal/cli already enforce in part. The gate flags shrink to a clause of the read-only item: no write operation is added without a product decision, which means reading the brief anyway. The note on the gate flags in the configuration precedence item repeated that item and is removed. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01HmjswYSvuZ7yb3erKTWJH3 --- CONTRIBUTING.md | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a3e911c..66e3230 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -74,24 +74,19 @@ if a change would break one, raise it before implementing. - PAT authentication only; no SSPI or NTLM. The PAT is entered interactively by `tfscli auth login` and never accepted as a command argument. -- Read-only in v1; write operations additionally depend on JSON output landing first. -- When write operations are added, every operation that changes server state requires - `--allow-changes`, and an irreversible one `--allow-irreversible` as well. Changing - server state is a property of the command, never of a flag of a reading command. The - gate flags have no short form, environment variable, or config key. +- Read-only in v1; write operations additionally depend on JSON output landing first, and + each will require `--allow-changes` (and `--allow-irreversible` if it cannot be undone), + as the brief describes. - Markdown is the default output format. - The command hierarchy and parameter names mirror the TFS REST API; a deviation needs an explicit usability justification, and no query syntax or batch endpoint is invented. - The command line stays matchable by agent permission rules: flags follow the command - path, each command has one form (no aliases, abbreviations, or prefix or - case-insensitive matching), a group of commands (an area, a resource, or a local group - such as `auth`) only groups them (no arguments, no flags of its own), local - command names never coincide with REST API areas, and there is no generic pass-through - command. + path, each command has one form, a group of commands only groups them, local command + names never coincide with REST API areas, and there is no generic pass-through command. - Stateless: no daemon, no background process, no on-disk state beyond the config file and the credential written by `auth login`. - The error categories `auth`, `not_found`, `forbidden`, `server`, `config`, and `network` are a contract. New ones may be added; existing ones are never renamed or removed. -- Configuration precedence is flag, environment variable, config file, built-in default. The server URL, the collection, and the PAT stay outside it: they come only from `auth.json` or `TFSCLI_AUTH`, together. The gate flags of write operations stay outside it too: they are given on the command line only. +- Configuration precedence is flag, environment variable, config file, built-in default. The server URL, the collection, and the PAT stay outside it: they come only from `auth.json` or `TFSCLI_AUTH`, together. **Dependencies.** Three direct dependencies — `cobra`, `html-to-markdown/v2`, and `golang.org/x/term` — are the intended total. HTTP is `net/http`; JSON and configuration From 5a9fd942126437b553b5c4138ab3564f92e90d56 Mon Sep 17 00:00:00 2001 From: Dmitry Pleshakov Date: Sat, 10 Oct 2026 09:20:21 +0300 Subject: [PATCH 23/25] Trim the command line rules in the brief and the architecture The cli entry in architecture.md retold the flag order check line by line: the quoting for bash and PowerShell, the repeated move of the path, cobra's completion command, pflag's wording, the details of the oracle test. The comments in internal/cli/flagorder.go already say all of it next to the code, so the entry keeps what the code does not: the check and why it needs no flag values, the tests that guard the tree shape, and the rejected alternatives. Both data flows now start with the check. The brief keeps the product decisions and drops cobra internals: the hidden completion command becomes one sentence on shell completion, and the OPTIONS detail of the gate, already in the architecture, goes. The gate flags are said to stand outside the configuration chain once, in "Configuration". Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01HmjswYSvuZ7yb3erKTWJH3 --- docs/architecture.md | 6 +++--- docs/project-brief.md | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index fdd9e1a..391ec8c 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -195,7 +195,7 @@ This section describes the conceptual module structure, their responsibilities, ### Modules and responsibilities -- **cli** — cobra commands, nested as area, resource, and action after the REST API reference (`wit` → `work-items` → `get`); persistent flags (`--verbose`, config overrides); orchestrates the chain config → apiclient → domain → printer → exit. Owns the markdown printer functions (one per resource and shape, such as a single work item and several) plus small per-kind scalar formatters for identity and datetime values used by those printers. No `Renderer` interface in v1 — printers are plain functions. When `--json` is added later, the interface and a second renderer can be introduced here without touching the domain layer. Enforces the command line shape that "The command line can be governed by permission rules" in `project-brief.md` requires. Cobra accepts flags at any position, so `run` checks the arguments before executing the tree: the path of the command must occupy exactly the first words of the command line, and otherwise the command fails with a `config` error that names the command and shows the corrected command — the path followed by every other word in its original order — when every word has a form that bash and PowerShell read alike. A word outside a safe set of characters is put in single quotes; `@` and `%` are outside it, since PowerShell splats a variable after `@` and stops parsing at `--%`. A word with a single quote, ASCII or typographic (PowerShell takes both for a quote), is put in double quotes, provided it holds none of the characters either shell interprets there: `"`, a typographic double quote, `$`, a backtick, `\`, or `!`. A word with a control character, such as a multi-line query, or with a single quote and one of those characters has no such form; the message then says to move the command to the start and keep every other word in its order. The command is the one reached from the root by taking, in order, every word that names a subcommand of the command reached so far; flag values are not read, and `--` does not stop the search, since pflag takes it for the value of a flag placed before it. When moving the path to the front turns a word that preceded one of its steps into a further step, as `login` in `tfscli - login auth`, the move is repeated, so that the corrected command is itself accepted. This depends on the shape of the tree: a group takes no arguments, a group below the root defines no flags, the root defines only persistent ones, and the flags cobra adds itself, `--help` and the root's `--version`, take no value, so a word naming a subcommand can follow the path only as the value of a persistent flag given to a group, which does nothing but print its help, and the rule reaches at least as deep as cobra's `Find`, which descends only through such words. A rule that read flag values to find the path was rejected: it is a second parser of the command line, and each of its divergences from pflag let a command run with its path not at the start. `newRoot` registers cobra's `help` and `completion` commands up front so that they are found like any other command. Cobra's hidden `__complete` command, also accepted as `__completeNoDesc`, which the shell completion scripts run, is added only when cobra executes the tree, so the check takes it for a subcommand of the root that ends the path: the words after it are the command line to complete. A flag before the path that the command does not define, or that is malformed, is reported as such, in pflag's wording, rather than moved into a corrected command that would fail the same way; the words before the path are read as pflag reads them, so a flag's value and the words after `--` are not taken for flags. This applies when the command is an action: a group ignores the flags it does not know, and before a group they are usually the flags of the intended action placed before a mistyped name, so the line gets the corrected command, on which cobra reports the mistyped name. `--version`, defined on the root alone, is refused before a path with a message that names `tfscli --version`. The check is explicit rather than a root `PersistentPreRunE`, because cobra runs only the nearest such hook and a child command defining its own would silently drop it. A test walks the whole command tree and fails on any command alias (cobra adds `__complete` only when it executes the tree, so its alias lies outside the test), on cobra's prefix or case-insensitive matching, and on a command with subcommands that accepts an argument or defines a flag other than a persistent flag of the root, so that each command keeps exactly one form and the check keeps the shape it relies on. Another test takes cobra as the oracle: over every command line of up to three words built from the names and flags of the tree and the words pflag treats specially, and a fixed-seed sample of longer lines, it requires that a line the check accepts makes cobra select a command whose path opens the line, that the corrected command proposed for a refused line is accepted, makes cobra select the command the message names, and leaves cobra no flag before the path to reject, and that a line refused for a flag before the path is rejected by cobra with the same message once the path is moved to the front, unless cobra first stops at a flag value it cannot parse, which the check does not read. The tests turn off cobra's check, on Windows, of whether the process was started from Explorer: it lists every process of the system before each execution, which would make this test take minutes. Every group of tfscli, local ones included, is built with `newGroupCmd`, and `newRoot` gives cobra's `completion` group the same handling through `makeGroup`, so that an unknown command below any group is an error rather than its help. +- **cli** — cobra commands, nested as area, resource, and action after the REST API reference (`wit` → `work-items` → `get`); persistent flags (`--verbose`, config overrides); orchestrates the chain config → apiclient → domain → printer → exit. Owns the markdown printer functions (one per resource and shape, such as a single work item and several) plus small per-kind scalar formatters for identity and datetime values used by those printers. No `Renderer` interface in v1 — printers are plain functions. When `--json` is added later, the interface and a second renderer can be introduced here without touching the domain layer. Enforces the command line shape that "The command line can be governed by permission rules" in `project-brief.md` requires. Cobra accepts flags at any position, so `run` checks the arguments before executing the tree: the command path must occupy the first words of the command line, and otherwise the command fails with a `config` error that shows the corrected command. The path is found by following the words that name subcommands, without reading flag values. This holds only while a group takes no arguments and no group but the root, whose flags are all persistent, defines flags, which a test over the whole command tree enforces together with the absence of aliases and of prefix or case-insensitive matching; a second test takes cobra as the oracle and requires that a line the check accepts makes cobra select a command whose path opens the line. A check that read flag values to find the path was rejected: it is a second parser of the command line, and each of its divergences from pflag would let a command run with its path not at the start. The check is explicit rather than a root `PersistentPreRunE`, because cobra runs only the nearest such hook and a child command defining its own would silently drop it. Every group, cobra's `completion` included, fails on an unknown command rather than printing its help (`newGroupCmd`, `makeGroup`). The details — quoting of the corrected command for bash and PowerShell, shell completion requests, flags the command does not define — are documented in `internal/cli/flagorder.go`. - **config** — resolves the credential (`URL`, `Collection`, and `PAT`; a PAT is issued for one collection) from `TFSCLI_AUTH` or `$XDG_DATA_HOME/tfscli/auth.json`, never from a mix of sources; loads the optional `$XDG_CONFIG_HOME/tfscli/config.json`, applies environment overrides (`TFSCLI_*`), applies cobra flag overrides; validates that all required fields are set for the command being run. Writes `auth.json` for `auth login` (mode `0600`, directory `0700`). Holds `URL`, `Collection`, `PAT`, `Project`, `APIVersion`, `InsecureSkipVerify`, `CABundle`. Never logs PAT. - **apiclient** — wraps `net/http`. Builds URLs from `Config.URL + Collection + path` and adds `api-version`. A configured `APIVersion` is sent as it is. Otherwise the version is negotiated per request, as the official SDKs do: `OPTIONS {URL}/{Collection}/_apis` lists every resource location with its versions, the caller names its location by id, and the request carries the location's released version, or `{maxVersion}-preview.{resourceVersion}` when it has none — the version the server chooses for a request without one. The response is not cached. A 401 or a transport failure on `OPTIONS` fails the request; any other failure sends it without a version and notes why through `Logger.Info`, and a refusal of that request for the missing version gets the settings to use in its message. A call without a location, the login check, neither negotiates nor sends a version. Sets `Authorization: Basic base64(":")` on every request. Configures TLS using `CABundle` (appended to system root pool) and `InsecureSkipVerify`. Hooks the logger via a `RoundTripper` — that transport is the single call site of `LogRequest`, so every outgoing request is logged exactly once and no other module logs HTTP traffic. Classifies HTTP outcomes into the stable error categories defined in the brief (`auth`, `not_found`, `forbidden`, `server`, `config`, `network`). - **workitem** — domain logic for Work Items: Get Work Item, Work Items - List, and Get Work Items Batch. Owns the `WorkItem`/`Field` types, the `BatchRequest`/`Batch` types for reading several work items, and the allowlist of fields known to contain HTML (`System.Description`, `Microsoft.VSTS.TCM.ReproSteps`, `Microsoft.VSTS.TCM.SystemInfo`, `Microsoft.VSTS.Common.AcceptanceCriteria`, …). After unmarshalling, tags each field with its `FieldKind`. For several work items, keeps the server's order and reports the requested ids the server did not return. Returns raw values — does not perform markdown conversion. @@ -294,7 +294,7 @@ type Result struct { ### Data flow: `tfscli wit work-items get -p MyProject 12345 --fields System.Title,System.Description` -1. cobra parses the command and flags. +1. `run` checks that the command path opens the command line (see **cli**); cobra then parses the command and flags. 2. `config.Load` takes `URL`, `Collection`, and `PAT` from `TFSCLI_AUTH` or `auth.json`, which must carry all three; reads the config file if present, overlays env vars, overlays bound flags. The command then checks that `Project` is supplied either by `-p`, env, or config default. 3. cli builds a `Logger` (noop or stderr depending on `--verbose`) and an `apiclient.APIClient`. The TLS config is derived from `CABundle` and `InsecureSkipVerify`. If verification is disabled, the logger emits one `[warn] TLS verification disabled` line. 4. cli calls `workitem.Get(ctx, client, project, id, fields)`. @@ -308,7 +308,7 @@ type Result struct { ### Data flow: `tfscli wit wiql query-by-wiql -p MyProject --team Web --query ""` -1. cobra parses the command and flags. A missing or blank `--query` and a query given as an argument are rejected locally with category `config`, before any request. +1. `run` checks that the command path opens the command line (see **cli**); cobra then parses the command and flags. A missing or blank `--query` and a query given as an argument are rejected locally with category `config`, before any request. 2. `config.Load` resolves the configuration as for `wit work-items get`. The project is optional for this command; `--team` without a project from any source is rejected locally with category `config`. 3. cli builds the `Logger` and the `apiclient.APIClient` as above and calls `wiql.QueryByWiql(ctx, client, req)`, setting `Top` and `TimePrecision` only for flags that were given. 4. `wiql.QueryByWiql` builds the path `{Project}/{Team}/_apis/wit/wiql`, leaving out the segments that are empty, and calls `client.Post` with the body `{"query": ...}` and the optional `$top` and `timePrecision` in the query string. A syntax error in the query comes back as HTTP 400 with the server's message, which `apiclient` reports in the `config` category. diff --git a/docs/project-brief.md b/docs/project-brief.md index 67321dd..04db121 100644 --- a/docs/project-brief.md +++ b/docs/project-brief.md @@ -43,8 +43,8 @@ An AI agent can call `tfscli`, read work item content, and use it meaningfully i - **Write operations.** v1 is strictly read-only. Write operations are a future consideration, gated by PAT scopes. JSON output (`--json`) is a prerequisite for write support, since markdown conversion is lossy and not round-trippable to HTML. When write operations are added, they are gated on the command line as follows, so that an AI agent host can require confirmation for every change with one permission rule: - Every operation that changes server state requires the flag `--allow-changes`. An irreversible operation requires both `--allow-changes` and `--allow-irreversible`: a rule on `--allow-changes` then covers every change, and a rule on `--allow-irreversible` additionally excludes the irreversible ones. Without a required flag the command fails with a `config` error that names the flag. - The classification follows the meaning of the operation, not its HTTP method: Get Work Items Batch and Query By Wiql are POST requests and read only, while queueing a build changes server state. An operation is irreversible when its effect cannot be undone by another operation of the REST API: deleting a work item to the recycle bin is reversible, since it can be restored, and destroying it is not. - - The gate flags are flags only, with no short form. No environment variable or config key supplies them, because a setting outside the command line is invisible to permission rules that match the command text; they are outside the precedence chain in "Configuration". - - The gate is checked before any request is sent, the `OPTIONS` request that negotiates the API version included. + - The gate flags are flags only, with no short form. No environment variable or config key supplies them, because a setting outside the command line is invisible to permission rules that match the command text. + - The gate is checked before any request is sent. - The gate covers server state only. Local commands that change local state, such as `auth login`, live under `auth` and are interactive. - The gate flags are not parameters of the REST API and so deviate from "Follow TFS API structure". The deviation is justified by safety: whether a command changes server state cannot be told from its name, since action names come from the API and form an open set, while some POST operations only read. - **MCP server mode.** tfscli is a CLI utility, not an MCP server. If MCP integration is needed, it can be wrapped externally. @@ -61,8 +61,8 @@ An AI agent can call `tfscli`, read work item content, and use it meaningfully i - A path parameter that identifies the resource is a positional argument: `wit work-items get 12345` for `_apis/wit/workitems/{id}`. The project, the team, and the collection, which scope the request rather than identify the resource, are not: the collection comes from the credential, the project from `-p` or its defaults, and the team from `--team`. - A query or body parameter is a flag with the API name in kebab-case and without a leading `$`: `ids` is `--ids`, `errorPolicy` is `--error-policy`, `$expand` is `--expand`. A flag that is not given is not sent, so the server's default applies. - **The command line can be governed by permission rules.** AI agent hosts, such as Claude Code and opencode, decide whether an agent may run a command by matching its text against allow, ask, and deny rules. tfscli keeps that text predictable: - - The command path opens the command line, and flags follow its last word: `tfscli wit work-items get --verbose 12345`, not `tfscli --verbose wit work-items get 12345`. A flag placed earlier is refused with a `config` error that shows the corrected command. `--help` follows the path like any other flag, and the built-in `help` and `completion` commands are commands like any other. The one exception is cobra's hidden `__complete` command, which the shell completion scripts run with the words typed so far after it: those words are the command line to complete, not a command that runs, so they may hold flags before a path. Only an invocation without a command path, such as `tfscli --help` or `tfscli --version`, has no path for a flag to precede; `--version` belongs to the root alone. - - Each command has exactly one form: no command aliases, no abbreviated command or flag names, no prefix or case-insensitive matching of command names. The hidden `__complete` command, which cobra also accepts as `__completeNoDesc`, is the one command with two names; it runs no command of tfscli. + - The command path opens the command line, and flags follow its last word: `tfscli wit work-items get --verbose 12345`, not `tfscli --verbose wit work-items get 12345`. A flag placed earlier is refused with a `config` error that shows the corrected command. `--help` follows the path like any other flag. The one exception is shell completion, which passes the words typed so far to a hidden command: they are the command line to complete, not a command that runs. Only an invocation without a command path, such as `tfscli --help` or `tfscli --version`, has no path for a flag to precede; `--version` belongs to the root alone. + - Each command has exactly one form: no command aliases, no abbreviated command or flag names, no prefix or case-insensitive matching of command names. - A command that has subcommands — an area, a resource, or a local group such as `auth` — only groups them: it takes no arguments, defines no flags of its own, and does nothing but print its help. Only a command without subcommands runs. A group that also acts on its own, as `git stash` and `git remote` do, is excluded: a rule written for the group would also cover every command below it, and the command path could no longer be told from the group's own arguments. - The names of local commands (`auth`, `licenses`, `help`, `completion`) never coincide with an area of the REST API reference. - Whether a command changes server state is a property of the command, never of its flags: a reading command does not gain a side effect through a flag. There is no generic pass-through command that sends an arbitrary request; should one be added, it falls under the gate flags described in "Non-Goals". From 892084a7af60f8b4cf9a07ae056f00355feb800c Mon Sep 17 00:00:00 2001 From: Dmitry Pleshakov Date: Sat, 10 Oct 2026 09:27:34 +0300 Subject: [PATCH 24/25] Limit tasks files to work that needs a plan The tasks skill described itself as the workflow for any work that starts now, so an agent asked to fix the findings of a branch review planned the fixes as a tasks file. This branch gained five such files on top of the one for the feature itself, each with a two-task plan that was never agreed with anyone, and TASK-01 and TASK-02 now refer to several files in one branch's history. A tasks file is now reserved for work that needs a plan: several tasks, a breakdown agreed with the user before the work starts, or work that may outlast one session. Review fixes, single fixes, and small requested changes are done directly and explained in their commit messages, even when the user leaves the details to the agent. A fix that turns into a change of approach gets a tasks file once the user has approved the decision. The rule is in the skill rather than in CONTRIBUTING.md because it does not depend on the project; the global copy of the skill carries the same text. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_015c62YtYEvUtpS8VvRmvRb4 --- .claude/skills/tasks/SKILL.md | 35 +++++++++++++++++++++++++++-------- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/.claude/skills/tasks/SKILL.md b/.claude/skills/tasks/SKILL.md index 56ae574..228046a 100644 --- a/.claude/skills/tasks/SKILL.md +++ b/.claude/skills/tasks/SKILL.md @@ -1,6 +1,6 @@ --- name: tasks -description: Universal task-file workflow — create a structured tasks file for work that starts now and execute it step by step, tracking what is done vs. pending in any project. No project-specific assumptions. +description: Universal task-file workflow — create a structured tasks file for work that starts now and needs a plan agreed with the user, and execute it step by step, tracking what is done vs. pending in any project. Not for fixing review findings, a single fix, or another change done directly. No project-specific assumptions. argument-hint: "[feature description to break down | path to a tasks file]" --- @@ -17,6 +17,21 @@ not the one you are reading, read it and follow it instead of this file. Where the project's own documentation (for example `CONTRIBUTING.md` or `CLAUDE.md`) sets a different convention, the project's convention wins over this skill. +## When a tasks file is warranted + +A tasks file holds a plan. Work gets one when it needs a plan: it breaks down into +several tasks, the breakdown is agreed with the user before the work starts, or the +work may outlast one session. + +Work that needs no plan is done directly, and its commit messages explain it. This +includes fixing the findings of a review of the current branch, a single fix, and a +small change the user asked for. Such work gets no tasks file even when the user +leaves the details to you, and even when it touches several files. + +When such work turns out to change the approach — the user has to approve a decision +before the work goes on — it has become work that needs a plan: agree the decision +with the user, then create a tasks file for the work that follows from it. + ## Mode detection Decide what to do from the user's request and the current context: @@ -25,8 +40,9 @@ Decide what to do from the user's request and the current context: and exactly one `Active` file exists in the tasks directory (see "The tasks file") → **Execute** mode. - A feature or request description is given and no matching file exists → **Create** - mode (breakdown), then offer to start executing. Work that is not to start now does - not get a tasks file (see "Create mode"). + mode (breakdown), then offer to start executing. Work that needs no plan (see "When + a tasks file is warranted") or is not to start now (see "Create mode") does not get + a tasks file. - The request names no new work and no `Active` file exists → propose starting the next item from where the project keeps accepted work that has not started, if it has such a place, as **Create** mode; otherwise ask the user what to work on. @@ -97,18 +113,20 @@ self-evident and needs no marker. A tasks file is created when the work starts, not in advance: most of the effort before that is discussion and decisions, and a breakdown made ahead of time goes stale. -1. Confirm that the work starts now. If it does not — the user wants it recorded, +1. Confirm that the work needs a plan (see "When a tasks file is warranted"). If it + does not, do it directly, without a tasks file. +2. Confirm that the work starts now. If it does not — the user wants it recorded, noted, or kept for later — do not create a tasks file. Record it where the project keeps accepted work that has not started, if it has such a place; otherwise ask the user where it should go. If the work was recorded there earlier, use that record as the input to the breakdown and take it out as that place's rules require. -2. Turn the feature/request into a complete, ordered list of **atomic** tasks — each +3. Turn the feature/request into a complete, ordered list of **atomic** tasks — each one achievable in a single focused session, with a definition of done that can be checked. -3. Propose the task structure to the user and let them adjust before writing anything. -4. Write the file to the tasks directory with header `Active` and every task `Pending`, +4. Propose the task structure to the user and let them adjust before writing anything. +5. Write the file to the tasks directory with header `Active` and every task `Pending`, numbered `TASK-01`, `TASK-02`, … in order. -5. Offer to start executing the first task. +6. Offer to start executing the first task. ## Execute mode (tracking discipline) @@ -161,6 +179,7 @@ same commit: ## Invariants +- A tasks file holds a plan; work that needs no plan gets no tasks file. - Tasks are atomic; work on one at a time. - Never leave a finished task unmarked — the file is the source of truth for progress. - `Done` tasks are never rewritten; `Pending` tasks change when the plan proves wrong. From 173424449ddc8021b001775e836056bdfb5814d4 Mon Sep 17 00:00:00 2001 From: Dmitry Pleshakov Date: Sat, 10 Oct 2026 09:27:48 +0300 Subject: [PATCH 25/25] Shorten the constraints repeated in CLAUDE.md CLAUDE.md is loaded into every agent session, so it names each rule and leaves the detail to the brief, as CONTRIBUTING does. The gate flags shrink to a clause of the read-only item, and the command line rule drops the expansions that the command tree tests enforce. The gate flags are no longer mentioned in the configuration precedence item, and the command shape section no longer repeats that flags follow the action. That no flag gives a reading command a side effect is a rule of the command line, in force before any write operation exists, and was lost from CONTRIBUTING together with the gate details; both files now state it with the other command line rules. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01HmjswYSvuZ7yb3erKTWJH3 --- CLAUDE.md | 9 ++++----- CONTRIBUTING.md | 3 ++- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index f2b1367..cfa36f3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -21,14 +21,13 @@ Read these before the corresponding kind of change (the full list of project doc These come from `project-brief.md` and override casual feature requests. If a change would violate one of these, surface it before implementing. - **PAT auth only.** No SSPI or NTLM. A PAT is issued for one collection, so the server URL, the collection, and the PAT are stored together and always come from one source: `$XDG_DATA_HOME/tfscli/auth.json` (default `~/.local/share/tfscli/auth.json`, mode `0600`, written only by the interactive `tfscli auth login`) or `TFSCLI_AUTH` holding the same JSON, which takes precedence. No flag, separate env var, or config key may supply the URL, the collection, or the token. No OS keychain unless an organisation requires one. -- **Read-only in v1.** Write operations are explicitly out of scope. They additionally depend on JSON output landing first, because markdown is lossy. -- **Write operations are gated on the command line.** When they are added, every operation that changes server state requires `--allow-changes`, and an irreversible one (its effect cannot be undone by another API operation) requires `--allow-irreversible` as well. Writing is a property of the command, never of a flag of a reading command. The gate flags have no short form, environment variable, or config key, and are checked before any request. See "Write operations" in Non-Goals of `project-brief.md`. -- **The command line stays matchable by agent permission rules.** Flags follow the last word of the command path; each command has one form (no aliases, no abbreviations, no prefix or case-insensitive matching); a group of commands (an area, a resource, or a local group such as `auth`) only groups them (no arguments, no flags of its own, nothing but its help); local command names never coincide with REST API areas; no generic pass-through command. See "The command line can be governed by permission rules" in `project-brief.md`. +- **Read-only in v1.** Write operations are explicitly out of scope. They additionally depend on JSON output landing first, because markdown is lossy, and each will require `--allow-changes` (and `--allow-irreversible` if it cannot be undone) — see "Write operations" in Non-Goals of `project-brief.md`. +- **The command line stays matchable by agent permission rules.** Flags follow the command path; each command has one form; a group of commands only groups them; local command names never coincide with REST API areas; no flag gives a reading command a side effect; no generic pass-through command. See "The command line can be governed by permission rules" in `project-brief.md`. - **Markdown is the default output.** HTML fields (Description, ReproSteps, etc.) are converted via `github.com/JohannesKaufmann/html-to-markdown/v2`. JSON output is a planned `--json` flag, not present in v1. - **Mirror TFS REST API structure.** Command hierarchy, parameter names, and behaviour follow the API. Any deviation needs explicit usability justification — don't invent new abstractions (no custom query syntax, no synthetic batch endpoints, no caching). - **Stateless.** No daemon, no background process, no on-disk cache. Every call hits the server. - **Stable error contract.** Error categories (`auth`, `not_found`, `forbidden`, `server`, `config`, `network`) are a contract: existing categories are never removed or renamed. New ones may be added. Format: `Error [category]: message (HTTP status)` to stderr, non-zero exit. -- **Config precedence:** CLI flag > env var (`TFSCLI_PROJECT`, `TFSCLI_API_VERSION`) > config file (optional) > built-in default. The credential and the gate flags of write operations are outside this chain (see above). +- **Config precedence:** CLI flag > env var (`TFSCLI_PROJECT`, `TFSCLI_API_VERSION`) > config file (optional) > built-in default. The credential is outside this chain (see above). - **Target is on-prem TFS / Azure DevOps Server; the API version is negotiated.** Without a configured version, each request is preceded by `OPTIONS {url}/{collection}/_apis` and carries the version the server lists for its resource; when the negotiation fails, the request goes out without a version, as before. A configured version is sent as is. `auth login` never sends one. Cloud Azure DevOps Services may work incidentally but is not tested or supported. - **Do not narrow server compatibility.** A change must not stop tfscli from working with a TFS / Azure DevOps Server version it worked with before, unless that is an explicitly discussed and recorded decision (see "Server compatibility is preserved" in `project-brief.md`). - **Works out of the box; errors lead the way.** Assume the user has read nothing and configured nothing. Do not add a required setting where a working default exists, and do not guess where the answer is ambiguous — ask the user instead. Every error a first-time user can hit names the concrete next step, and says when only a person can take it (see "Works out of the box; errors lead the way" in `project-brief.md`). @@ -40,7 +39,7 @@ These come from `project-brief.md` and override casual feature requests. If a ch tfscli [flags] [arguments] ``` -The area and the resource are the segments of the operation's page path in the REST API reference (`.../wit/work-items/get-work-item` is `wit work-items get`); the action is the operation name without the resource name. Flags follow the action: a flag placed before the end of the command path, such as `tfscli --verbose wit work-items get 1`, is refused. `tfscli auth login` and `tfscli licenses` are local commands and lie outside this scheme. The full rule is in "Follow TFS API structure" in `project-brief.md`. +The area and the resource are the segments of the operation's page path in the REST API reference (`.../wit/work-items/get-work-item` is `wit work-items get`); the action is the operation name without the resource name. `tfscli auth login` and `tfscli licenses` are local commands and lie outside this scheme. The full rule is in "Follow TFS API structure" in `project-brief.md`. Commands (from the brief): diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 66e3230..538f61a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -82,7 +82,8 @@ if a change would break one, raise it before implementing. explicit usability justification, and no query syntax or batch endpoint is invented. - The command line stays matchable by agent permission rules: flags follow the command path, each command has one form, a group of commands only groups them, local command - names never coincide with REST API areas, and there is no generic pass-through command. + names never coincide with REST API areas, no flag gives a reading command a side + effect, and there is no generic pass-through command. - Stateless: no daemon, no background process, no on-disk state beyond the config file and the credential written by `auth login`. - The error categories `auth`, `not_found`, `forbidden`, `server`, `config`, and `network` are a contract. New ones may be added; existing ones are never renamed or removed.