Multiple improvements in the CLI - #4
Merged
Merged
Conversation
No list command set Args, so Cobra fell through to ArbitraryArgs and `timestripe goals list --checked false` parsed as --checked=true, silently discarding "false" and returning completed goals with exit code 0. SKILL.md teaches the --checked=false form, one keystroke from the broken one. addListFlags already runs on every list command, so setting cmd.Args there closes the hole in one place. A bool literal in the positional slot gets a targeted hint naming the flags it could have meant. Also: - `folders goals add` had no Args validator either. - On `create [name]` the positional was applied after the flag and silently won over --name (on `comments create` the flag won). Reject the ambiguity rather than document which way it falls. - The generated client used http.DefaultClient, which has no timeout. Bound a single request at 60s. - main called root.Execute(), so cmd.Context() was context.Background() and Ctrl-C could not cancel an in-flight --all walk. Thread a signal context through and exit 130 on cancellation. - Split newRootCmd() out of Execute() so tests can drive the tree. - Add CI: gofmt, vet, test, build. Only release.yml existed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
apiError dumped the raw response body, so a validation failure read as
`api returned status 400: {"horizon":["\"weekly\" is not a valid choice."]}`
and a 500 from a proxy could paste a full Django debug page into the terminal.
The API is DRF, which returns a small set of predictable shapes. Parse them:
{"detail":…}, non_field_errors, per-field maps, bare arrays, and one level of
list-serializer nesting flattened to magic_links.0.role. Fields are sorted so
output does not vary with map iteration order. Unrecognized bodies are
whitespace-collapsed and capped at 500 chars; an HTML body is diagnosed as a
proxy or bad TIMESTRIPE_BACKEND rather than printed.
401 and 403 now say what to run. A 400 naming an *_id field that does not
exist points at the ref flags, which accept names as well as IDs.
The apiError(status, body) signature is unchanged, so all ~40 call sites
compile as-is and now return an *APIError.
--verbose/-v logs method, URL, status, duration, and the raw body to stderr;
-v was already reserved for this.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The refresh token was persisted at login and never used: Resolve hard-errored on expiry with "run `timestripe auth login`", so every session ended in a manual browser round trip. Resolve now refreshes transparently. AuthStyleInParams is set explicitly — the CLI is a public PKCE client with no secret, so client_id belongs in the body, and leaving oauth2 to probe-and-cache the style makes the first refresh non-deterministic. Backend is preserved across the refresh; it is set at login and is not part of the token response. A rotated refresh token is stored; an omitted one leaves the existing token in place. Concurrency is the reason for the lockfile. Agents run several invocations at once, and if the server rotates refresh tokens, two simultaneous refreshes lose one permanently. Take an O_CREATE|O_EXCL lock with a 2s bounded spin and a 30s stale breaker, then re-read credentials after acquiring it — the other process has usually already refreshed, so the second caller does no network round trip at all. Failing to lock is not fatal: a possible double refresh beats a hang. `auth status` now reports time remaining and whether auto-refresh applies. Tests cover the refresh, persistence, Backend preservation, a cache hit on the second call, 8 concurrent Resolves collapsing to one token exchange, a stale lock being broken, and bearer tokens never expiring. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Name resolution fetched the ENTIRE resource list to match one name, so `--parent "Some goal"` on a large account walked every goal. Use the server-side search filter instead: a name now costs two requests rather than 1 + ceil(N/50) pages. /folders/ has no search param, so folders keep the full walk; that is what the searchable flag records. Three correctness fixes fall out: - getByID swallowed every error, so a 401, 403, or network failure fell through to a name scan and reported `no space matches "spc_abc"` — hiding an expired token behind a wrong diagnosis. Only a 404 now means "not an ID". - A value containing whitespace is never an ID, so skip the probe and its guaranteed 404. - Ambiguity errors listed only a count; they now name the candidates. When stdin is a terminal, an ambiguous or near-miss reference prompts with a numbered list instead of erroring. The TTY check is the contract: piped or redirected stdin never prompts and never blocks, so scripts, CI, and the agent skill get exactly the error they got before. Candidates carry a qualifier (a goal's horizon and date, a bucket's board) so identical names are distinguishable. Restructured around a resource[T] descriptor: each resource supplies one get-by-ID and one paginated list call rather than three near-identical closures, which removes roughly a third of the file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Filtering for unfinished goals required `--checked=false`, and the bare `--checked false` was silently misparsed until the previous fix made it an error. Neither is a good way to ask for "the things I still have to do". --open and --done say it directly. --checked keeps working and its help text now flags the "=" requirement. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Only -h existed. Every invocation spelled out --space, --bucket, --horizon. One rule: a letter means the same thing everywhere, or it gets no letter. -f --file -n --name -s --space -b --bucket -a --assignee -H --horizon -d --date -q --search -l --limit -A --all The awkward pairs are deliberate. -s is space, never search — `spaces list` has --search but no --space, so -s is simply absent there rather than meaning something else; -q carries search everywhere. -b is bucket, never board: goals have --bucket and no --board while buckets have the reverse, so -b-for-board would be locally free but globally inconsistent, and --board is not a hot path. -H and -A are capitalised because -h is help and -a is assignee. --board, --archived, --offset, --parent, --color, --description, --sort, --email, --type, --layout, --date-from and --date-to get no letter rather than an inconsistent one. The format selectors keep long form: they live in scripts, where clarity wins. A test walks the whole command tree and fails if any letter is ever bound to two different long names, if a flag drifts from the documented scheme, or if an undocumented shorthand appears. That guard, not the convention, is what keeps this from rotting. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Natural dates. --date now takes today, tomorrow, friday, +3d, -1w, +2m
alongside YYYY-MM-DD, in a new internal/dates package with an injected clock.
A bare weekday means the next occurrence INCLUDING today, so typing "friday"
on a Friday means today rather than a week out; "next friday" is strictly
after and "last friday" strictly before. +1m from Jan 31 clamps to Feb 28
rather than overflowing into March as time.AddDate does.
Three call sites, not one. The list filters went through dateFlag/timeFlag,
but --date on create and update passed the raw string straight through with no
validation at all, and --start-time/--end-time are format:time ("Time in
24-hour HH:MM format"), not datetimes — they were unvalidated too, with help
text that just said "start time".
"none" now clears a nullable field. date, horizon, color, bucket_id, parent_id
and assignee_id are all nullable in the schema, but there was no way to clear
one from a flag: --horizon "" sends an empty string and is rejected.
Enum validation runs at the top of RunE, before newAPIClient, so a typo costs
no round trip and no credentials. "weekly" now suggests "week" instead of
returning a raw 400. The tables are hand-written because the generated
constants are unusable — bare api.Day, api.Hash278dea, api.MinusDatetime, and
members like GoalHorizonLessThannil = "<nil>". A test re-parses
api/openapi.yaml and fails if any table drifts, and a second test fails if the
spec grows an enum with no table, so `make gen` cannot diverge silently.
Static completion for every enum flag, with a test that catches a missing
registration — RegisterFlagCompletionFunc's error is discarded, so a dropped
one is otherwise invisible until someone presses TAB.
Quick verbs: `add`, `done <goal>`, `reopen <goal>`, also available under
`goals`. done and reopen take a name or an ID, so `timestripe done "Buy milk"`
works with no lookup first. add is built by calling newGoalsCreateCmd() and
renaming it, so the two cannot drift; a test asserts their flag surfaces stay
identical. Root help is grouped into Quick/Resources/Setup.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Corrections. Every one of these described something that does not exist: - README quick start used --title, which is not a flag; it is --name or the positional. It also used `boards list --space`, which is --space-id. - README's jq example was wrong three ways: the output is an object with .items, not an array, and the fields are checked and name, not completed and title. - README claimed most commands have full CRUD; events, memberships and users are read-only. - SKILL.md claimed OAuth uses a fixed loopback port 53682; the code binds 127.0.0.1:0 and lets the OS choose. - SKILL.md claimed tokens go to the keychain; they go to a 0600 JSON file. - SKILL.md presented create/update as --file-only, omitting the entire flag surface and the name-or-ID resolution behind it — the biggest ergonomic feature in the CLI. An agent following the skill would never have found it. - SKILL.md said goal completion is "intentionally not exposed"; it is, and it now has dedicated verbs. - SKILL.md taught --checked=false, now replaced by --open. New material: a Shortcuts section covering the quick verbs, the short-flag table, name-instead-of-ID resolution, the date grammar, --open/--done, and enum completion. Verified by walking every `timestripe ...` command path in both documents against the built binary. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Correctness
--checked falseused to silently mean--checked=trueand return the wrong goals with exit 0.create [name]no longer lets the positional silently override--name; the ambiguity is an error.http.DefaultClient, no timeout).mainso Ctrl-C cancels an in-flight--allwalk; exit 130.release.ymlexisted.Auth
auth statusreports time remaining and whether auto-refresh applies.Errors
*_idpoints at the ref flags.--verbose/-v: method, URL, status, duration, and raw body to stderr.Reference resolution
getByIDno longer swallows errors, so an expired token stops reporting asno space matches "spc_abc".resource[T]descriptor, cutting ~⅓ of the file.Ergonomics
--open/--donetogoals list, replacing the awkward--checked=false.-f -n -s -b -a -H -d -q -l -A), guarded by a test that walks the whole command tree.today,friday,+3d,+2m) in a newinternal/datespackage, applied to the two create/update sites that had no validation at all.noneto clear nullable fields, which previously had no clearing path.weeklysuggestsweek.add,done <goal>,reopen <goal>, and grouped root help into Quick/Resources/Setup.Docs
--title, port 53682, keychain storage, "full CRUD") and documented the new shortcuts.