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. diff --git a/CHANGELOG.md b/CHANGELOG.md index 70e6c81..357b80d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,13 @@ 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 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` 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 ### Added diff --git a/CLAUDE.md b/CLAUDE.md index 3157e46..cfa36f3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -21,7 +21,8 @@ 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. +- **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. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ccb487f..538f61a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -74,10 +74,16 @@ 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. +- 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, 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, 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. diff --git a/README.md b/README.md index 9937aee..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,23 +128,75 @@ 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 + +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. + +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 +{ + "permissions": { + "allow": [ + "Bash(tfscli wit *)", + "Bash(tfscli licenses)", + "PowerShell(tfscli wit *)", + "PowerShell(tfscli licenses)" + ], + "ask": [ + "Bash(tfscli *--allow-changes*)", + "PowerShell(tfscli *--allow-changes*)" + ], + "deny": [ + "Bash(tfscli auth *)", + "PowerShell(tfscli auth *)" + ] + } +} +``` + +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 +{ + "permission": { + "bash": { + "tfscli wit *": "allow", + "tfscli licenses": "allow", + "tfscli *--allow-changes*": "ask", + "tfscli auth *": "deny" + } + } +} +``` ## Configuration @@ -213,6 +259,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; 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. ### `wit work-items get` diff --git a/docs/architecture.md b/docs/architecture.md index 0db7ab5..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. +- **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. @@ -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. --- diff --git a/docs/archive/2026-10-08-tasks-command-path-walk.md b/docs/archive/2026-10-08-tasks-command-path-walk.md new file mode 100644 index 0000000..2267161 --- /dev/null +++ b/docs/archive/2026-10-08-tasks-command-path-walk.md @@ -0,0 +1,77 @@ +# 2026-10-08-tasks-command-path-walk.md + +**Status:** Archived + +## 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:** Done diff --git a/docs/archive/2026-10-08-tasks-command-shape-permissions.md b/docs/archive/2026-10-08-tasks-command-shape-permissions.md new file mode 100644 index 0000000..abd9ef2 --- /dev/null +++ b/docs/archive/2026-10-08-tasks-command-shape-permissions.md @@ -0,0 +1,130 @@ +# 2026-10-08-tasks-command-shape-permissions.md + +**Status:** Archived + +## 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:** Done + +### 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:** Done + +### 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:** Done + +### 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:** Done + +### 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:** Done diff --git a/docs/archive/2026-10-09-tasks-flag-order-oracle.md b/docs/archive/2026-10-09-tasks-flag-order-oracle.md new file mode 100644 index 0000000..1b68ed9 --- /dev/null +++ b/docs/archive/2026-10-09-tasks-flag-order-oracle.md @@ -0,0 +1,61 @@ +# 2026-10-09-tasks-flag-order-oracle.md + +**Status:** Archived + +## 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:** Done diff --git a/docs/archive/2026-10-09-tasks-flag-order-review.md b/docs/archive/2026-10-09-tasks-flag-order-review.md new file mode 100644 index 0000000..1f7e089 --- /dev/null +++ b/docs/archive/2026-10-09-tasks-flag-order-review.md @@ -0,0 +1,77 @@ +# 2026-10-09-tasks-flag-order-review.md + +**Status:** Archived + +## 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:** Done diff --git a/docs/archive/2026-10-09-tasks-flag-order-second-review.md b/docs/archive/2026-10-09-tasks-flag-order-second-review.md new file mode 100644 index 0000000..181ee23 --- /dev/null +++ b/docs/archive/2026-10-09-tasks-flag-order-second-review.md @@ -0,0 +1,91 @@ +# 2026-10-09-tasks-flag-order-second-review.md + +**Status:** Archived + +## 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:** Done + +### 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:** Done + +### 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:** Done + +### 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:** Done diff --git a/docs/archive/2026-10-09-tasks-simple-path-rule.md b/docs/archive/2026-10-09-tasks-simple-path-rule.md new file mode 100644 index 0000000..78a2a43 --- /dev/null +++ b/docs/archive/2026-10-09-tasks-simple-path-rule.md @@ -0,0 +1,65 @@ +# 2026-10-09-tasks-simple-path-rule.md + +**Status:** Archived + +## 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:** Done diff --git a/docs/project-brief.md b/docs/project-brief.md index abd9888..04db121 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. + - 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. - **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,12 @@ 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. `--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". - **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 +100,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: 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.go b/internal/cli/cli.go index 521f428..7f41c77 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) @@ -97,37 +101,47 @@ 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() + 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 fa656e6..8103414 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. @@ -581,6 +593,21 @@ 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: "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"}, + 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 new file mode 100644 index 0000000..88d282a --- /dev/null +++ b/internal/cli/flagorder.go @@ -0,0 +1,279 @@ +package cli + +import ( + "errors" + "fmt" + "slices" + "strings" + "unicode" + + "github.com/spf13/cobra" + + "github.com/dpleshakov/tfscli/internal/tfserr" +) + +// 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 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, 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 +// that ends the path: the words after it are the command line to complete. +func checkFlagOrder(root *cobra.Command, args []string) error { + cmd, name, corrected, before := reorder(root, args) + if corrected == nil { + return 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 + } + 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 (%s)", name, next), + } +} + +// 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 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) { + // 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) + } + } + order = next + } + + 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 +} + +// 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()} + for i, arg := range line { + if cmd == root && isCompletionRequest(arg) { + 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) + } + } + 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() { + if c.Name() == name { + return c + } + } + return nil +} + +// unknownFlag reports the first flag among words, read as pflag reads them, +// 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] + 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), + Cause: errVersionFlag, + } + 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++ + } + } + } + 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 +// 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, group[:1], true) + switch { + case !found: + 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 false, nil +} + +// 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 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.ContainsAny(a, singleQuotes): + a = "'" + a + "'" + case !strings.ContainsAny(a, doubleQuoted): + a = `"` + a + `"` + default: + return "", false + } + quoted[i] = a + } + return strings.Join(quoted, " "), true +} + +// 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 new file mode 100644 index 0000000..8dc7b34 --- /dev/null +++ b/internal/cli/flagorder_test.go @@ -0,0 +1,546 @@ +package cli + +import ( + "errors" + "fmt" + "io" + "math/rand/v2" + "net/http" + "regexp" + "runtime" + "slices" + "strings" + "sync" + "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 +// 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" +} + +// 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) { + 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: 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: 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: 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: 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: 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"}, + 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: 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: 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: 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: refused("tfscli wit work-items get", "tfscli wit work-items get -p get 12345"), + }, + { + 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: "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"}, + want: refused("tfscli help", "tfscli help --verbose wit"), + }, + { + name: "flag before completion", + 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: "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"}, + 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"}, + 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 { + 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) + } + }) + } +} + +// 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"}, 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", "wiql", "query-by-wiql", "-h"}, want: "--time-precision"}, + {args: []string{"help", "wit", "work-items", "get"}, want: "--fields"}, + {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 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) + } + }) + } +} + +// 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) + } + }) + } +} + +// 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, 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) { + 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() { + // 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 != "" { + failures <- msg + } + } + }) + } + for _, line := range lines { + next <- line + } + close(next) + wg.Wait() + close(failures) + + var msgs []string + for msg := range failures { + 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 properties of +// TestFlagOrderAgreesWithCobra and describes the first one it breaks. +func checkAgainstCobra(root *cobra.Command, line []string) string { + shown := fmt.Sprintf("tfscli %q", line) + 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 && isCompletionRequest(line[0]) { + 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) + } + + var te *tfserr.Error + if !errors.As(err, &te) { + return fmt.Sprintf("%s: refused with an error of no category: %v", shown, err) + } + 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. + leaf := !cmd.HasSubCommands() + + if !strings.Contains(te.Message, "must come first") { + 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"): + 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 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 != "" && !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. +func selected(args []string) string { + root := stubbedRoot() + root.SetArgs(args) + cmd, _ := root.ExecuteC() + 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 its value as invalid, 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: ", invalidValue} { + if strings.HasPrefix(err.Error(), kind) { + return err.Error() + } + } + return "" +} + +// 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{ + "--", "-", "", "--=X", "--help", "-h", "--version", "--bogus", "-z", "X", "5", + cobra.ShellCompRequestCmd, cobra.ShellCompNoDescRequestCmd, + } + 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", "-"+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. 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 new file mode 100644 index 0000000..c7289e8 --- /dev/null +++ b/internal/cli/tree_test.go @@ -0,0 +1,61 @@ +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, 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) { + 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)) +} + +// 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. 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) { + 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()) + } + 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() { + walk(child) + } + } + walk(newRoot(testBuild, noTerminal{}, io.Discard, io.Discard)) +} diff --git a/skills/tfscli/SKILL.md b/skills/tfscli/SKILL.md index b2ac6db..29b3861 100644 --- a/skills/tfscli/SKILL.md +++ b/skills/tfscli/SKILL.md @@ -131,6 +131,15 @@ 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 (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. + ### One work item ```