From ac2ebdba3996811a260161f7020ea04226a323cc Mon Sep 17 00:00:00 2001 From: dlq Date: Wed, 13 May 2026 12:52:30 -0400 Subject: [PATCH 1/5] Add CLI improvement planning docs --- plan.md | 373 ++++++++++++++++++++++++++++++++++++++++ summer-student-scope.md | 117 +++++++++++++ 2 files changed, 490 insertions(+) create mode 100644 plan.md create mode 100644 summer-student-scope.md diff --git a/plan.md b/plan.md new file mode 100644 index 0000000..7140e89 --- /dev/null +++ b/plan.md @@ -0,0 +1,373 @@ +# CBRAIN CLI Improvement Backlog + +This is the full technical backlog. It is intentionally broader than one summer project. + +For the student-facing 3-month scope, use `summer-student-scope.md`. That document explains which parts of this backlog are required, which are optional, and which are out of scope. + +## Priority Model + +- **Phase 1:** user-visible correctness bugs. +- **Phase 2:** low-hanging guardrails that make future work safer. +- **Phase 3:** output and UX consistency. +- **Phase 4:** incremental architecture cleanup. +- **Phase 5:** documentation and compatibility. + +# Phase 1: Correctness Fixes + +## 1. Fix `task list bourreau-id ` filtering + +**Problem:** The parser accepts `bourreau-id`, but the data code checks `bourreau_id`, so the filter is ignored. + +**Do:** +- Keep the public CLI spelling as `bourreau-id`. +- Use argparse `dest=` or normalization so internal code sees `bourreau_id`. +- Add coverage proving the generated request includes `bourreau_id=...`. + +**Verify:** `cbrain task list bourreau-id 3` sends the expected filter. + +## 2. Stop execution after invalid pagination + +**Problem:** `pagination()` returns `None` for invalid input, but callers continue. Some crash; others make the wrong request. + +**Do:** +- Give pagination a clear contract: raise a validation error or return a handled sentinel. +- Apply the same behavior to all paginated list commands. +- Ensure invalid pagination makes no network request. + +**Verify:** `--per-page 1` and `--page 0` return a clear error and non-zero exit. + +## 3. Fix `tool show ` lookup + +**Problem:** `tool show` fetches only the first `/tools` page and filters client-side, so valid tools outside page 1 can appear missing. + +**Do:** +- Prefer a direct `/tools/{id}` request if available. +- Otherwise fetch deliberately and document the limitation. +- Split `tool list` and `tool show` data functions. + +**Verify:** A tool outside the first default page can be shown correctly. + +## 4. Allow `logout` to clean up invalid credentials + +**Problem:** `main()` blocks `logout` behind `is_authenticated()`, making cleanup unreachable for malformed credentials. + +**Do:** +- Dispatch `logout` without requiring authentication. +- Make local credential removal robust when the file is missing or malformed. + +**Verify:** A malformed credentials file is removed by `cbrain logout`. + +## 5. Remove import-time config directory creation + +**Problem:** `config.py` creates `~/.config/cbrain` at import time, so read-only commands can fail before parsing. + +**Do:** +- Keep credential paths as constants. +- Create directories only when writing credentials. +- Ensure reads tolerate missing files/directories. + +**Verify:** `cbrain --help` and `cbrain version` do not write to `HOME`. + +## 6. Print empty list results consistently + +**Problem:** Handlers using `if result:` suppress valid empty lists. + +**Do:** +- Use `is not None` for list command results. +- Let formatters handle empty-list messages. +- Ensure JSON mode emits `[]`. + +**Verify:** Empty API list responses produce useful normal output and valid JSON/JSONL behavior. + +## 7. Implement or remove `project unswitch` + +**Problem:** `project unswitch` is advertised but only prints a not-implemented message. + +**Do:** +- Prefer implementing it by clearing `current_group_id` and `current_group_name`. +- Make it idempotent when no project is set. +- Respect `--json` and `--jsonl`. + +**Verify:** `project switch`, `project unswitch`, and `project show` behave coherently. + +# Phase 2: Low-Hanging Guardrails + +## 8. Enforce Ruff linting and formatting + +**Problem:** Ruff is configured but not clearly enforced as part of routine development. + +**Do:** +- Standardize `ruff check .` and `ruff format .`. +- Add pre-commit and/or CI enforcement. +- Document the commands. + +**Verify:** CI or local checks fail on lint/format drift. + +## 9. Add unit tests beside capture tests + +**Problem:** Capture tests protect output but do not isolate parsing, validation, request construction, or handler contracts. + +**Do:** +- Add a minimal unit test harness, preferably `pytest`. +- Cover Phase 1 regressions first. +- Mock `urllib.request.urlopen` for request tests. + +**Verify:** `python -m pytest` runs without a live CBRAIN server. + +## 10. Add regression tests for falsey valid results + +**Problem:** Empty lists and possibly empty dictionaries can be valid responses. + +**Do:** +- Test empty list handling for each list handler. +- Decide explicitly whether empty dictionaries are valid for show commands. + +**Verify:** Empty successful responses do not disappear. + +## 11. Add HTTP timeouts + +**Problem:** `urlopen()` calls have no timeout and can hang indefinitely. + +**Do:** +- Add a default timeout, for example 30 seconds. +- Make timeout errors use shared error handling. +- Optionally allow environment or config override later. + +**Verify:** Mocked timeout errors return clear messages and non-zero status. + +## 12. Protect credentials file permissions + +**Problem:** API tokens are stored as plain JSON without explicit permission handling. + +**Do:** +- Create credential files with user-private permissions where supported. +- Preserve restrictive permissions when updating. +- Handle non-POSIX platforms gracefully. + +**Verify:** On POSIX, credentials are written with private permissions. + +## 13. Use one source of truth for versioning + +**Problem:** `version_info()` hardcodes `1.0`, while repository tags may differ. + +**Do:** +- Put version metadata in one place. +- Make `cbrain version` read package metadata when installed. +- Provide a source-tree fallback. + +**Verify:** Reported version matches package metadata and release tags. + +## 14. Add a contributor checklist + +**Problem:** Review expectations are implicit. + +**Do:** +- Add a short checklist to `README.md` or `CONTRIBUTING.md`. +- Include Ruff, tests, capture tests, output modes, data-layer printing, and credential safety. + +**Verify:** Contributors can find the checklist before opening PRs. + +## 15. Treat linting as a baseline + +**Problem:** A clean Ruff run does not prove command behavior, output modes, or architecture are correct. + +**Do:** +- Document that linting complements tests and review. +- Keep architecture-sensitive checks in the review checklist. + +**Verify:** CI includes both lint/format checks and tests. + +# Phase 3: Output and UX Consistency + +## 16. Audit global `--json` and `--jsonl` behavior + +**Problem:** Several commands ignore global output flags or always print JSON/plain text. + +**Known examples:** `version`, `tool-config boutiques-descriptor`, `dataprovider is-alive`, `dataprovider delete-unregistered-files`, `project switch all`, file operations, and tag operations. + +**Do:** +- Create a command/output matrix. +- Convert operation commands to structured result dictionaries. +- Route machine-readable output through `json_printer()` and `jsonl_printer()`. + +**Verify:** Representative commands work in normal, JSON, and JSONL modes. + +## 17. Make not-implemented behavior structured + +**Problem:** Not-implemented paths print prose and return inconsistently. + +**Known examples:** `project switch all`, `project unswitch`, `task operation`. + +**Do:** +- Decide whether each command should exist. +- If it remains, return non-zero and emit structured JSON/JSONL errors. + +**Verify:** Not-implemented commands behave consistently in all output modes. + +## 18. Stabilize command vocabulary + +**Problem:** Public terms and option names are inconsistent: `dataprovider`, `remote-resource`, `bourreau-id`, `dp-id`, `data-provider`, etc. + +**Do:** +- Decide canonical public terms. +- Keep backward-compatible aliases where practical. +- Improve help text for CBRAIN-specific terms. + +**Verify:** `cbrain --help` and subcommand help read consistently. + +## 19. Clarify destructive command safety + +**Problem:** Delete and cleanup commands need clear confirmation, force, partial success, and script behavior. + +**Do:** +- Decide which destructive commands require confirmation. +- Add `--yes` or `--force` where appropriate. +- Return structured operation results for scripts. + +**Verify:** Destructive commands behave predictably in interactive and non-interactive contexts. + +## 20. Add coherent verbose/debug mode + +**Problem:** Debug behavior is ad hoc and mostly tied to `whoami --version`. + +**Do:** +- Add a global `--verbose` or `--debug`. +- Print sanitized method/path/status diagnostics. +- Never print raw tokens or passwords. + +**Verify:** Debug output is useful and credential-safe. + +# Phase 4: Incremental Architecture Cleanup + +## 21. Define command return contracts + +**Problem:** Functions return mixed values: `None`, `1`, lists, dicts, and tuples. + +**Do:** +- Data functions return domain data or raise typed errors. +- Handlers orchestrate formatting and return exit codes. +- Formatters only print. +- Apply to one command family first. + +**Verify:** Handler tests cover success, empty data, validation errors, and API errors. + +## 22. Add typed CLI exceptions + +**Problem:** Broad exception handling hides intent and makes failures inconsistent. + +**Do:** +- Add exceptions such as `CliValidationError`, `CliApiError`, and `CliResponseError`. +- Update shared error handling to treat known errors clearly. + +**Verify:** Expected failures get stable messages; unexpected failures still return non-zero. + +## 23. Move printing out of data modules + +**Problem:** Data modules mix API work, validation, and presentation by printing directly. + +**Do:** +- Replace direct data-layer `print()` calls with typed errors or structured results. +- Keep user-facing output in handlers/formatters. + +**Verify:** `cbrain_cli/data` has no new direct user-facing output. + +## 24. Remove ad hoc background activity error handling + +**Problem:** `list_background_activities()` catches all exceptions and bypasses shared error handling. + +**Do:** +- Remove broad `except Exception`. +- Let shared handling process HTTP, URL, JSON, and unexpected errors. + +**Verify:** Background activity failures match other command error styles. + +## 25. Normalize CLI argument names before data-layer use + +**Problem:** Raw public CLI spellings leak into data modules, causing bugs like `bourreau-id` vs `bourreau_id`. + +**Do:** +- Use argparse `dest=` for normalized snake_case names. +- Translate API-specific field names at the API boundary. + +**Verify:** Parsed args and generated API keys are both tested. + +## 26. Split parser construction from execution + +**Problem:** `main()` builds the parser and executes commands, making parser behavior harder to test. + +**Do:** +- Extract `build_parser()`. +- Keep `main(argv=None)` for parse/auth/dispatch. + +**Verify:** Parser tests can run without API calls. + +## 27. Introduce a small CBRAIN API client + +**Problem:** Data modules duplicate request creation, auth headers, JSON decoding, and error handling. + +**Do:** +- Add a lightweight standard-library `CbrainClient`. +- Store base URL, token, user ID, and timeout on the client. +- Migrate one command family first. + +**Verify:** Client tests mock `urlopen`; migrated commands still pass capture tests. + +## 28. Centralize HTTP request construction + +**Problem:** Request construction is duplicated and inconsistent. + +**Do:** +- Centralize `get/post/put/delete`, URL joining, auth headers, JSON parsing, and HTTP error wrapping. +- If `CbrainClient` exists, put this logic there. + +**Verify:** URL generation and JSON handling are unit tested. + +## 29. Load authentication state at command execution time + +**Problem:** Credentials are read into module globals at import time and can become stale. + +**Do:** +- Add `load_credentials()`. +- Stop importing static `api_token`, `cbrain_url`, and `user_id` into data modules. +- Prefer passing credentials through the API client. + +**Verify:** Login/logout/whoami behavior uses current credential state in one process. + +# Phase 5: Documentation and Compatibility + +## 30. Define supported CBRAIN API compatibility + +**Problem:** Endpoint and response assumptions are spread across the codebase. + +**Do:** +- Document the supported/tested CBRAIN API or server version. +- Centralize endpoint paths where practical. +- Add tests for representative response shapes. + +**Verify:** README states the compatibility target. + +## 31. Keep capture tests, but add faster isolated tests + +**Problem:** Capture tests are valuable but fragile as the only safety net. + +**Do:** +- Keep capture tests for end-to-end output. +- Use unit tests for parsing, validation, request construction, and formatter behavior. +- Run unit tests on every CI job. + +**Verify:** Request bugs fail unit tests; output regressions fail capture tests. + +## 32. Document internal boundaries + +**Problem:** The intended layering is implicit. + +**Do:** +- Add a short architecture section: + - `main.py`: parser and dispatch. + - `handlers.py`: orchestration and exit codes. + - `data/`: API calls and domain data. + - `formatter/`: user-facing output. + - `cli_utils.py` or `api.py`: shared helpers and errors. +- State that data modules should not print. + +**Verify:** Contributor docs match the architecture used by migrated command families. diff --git a/summer-student-scope.md b/summer-student-scope.md new file mode 100644 index 0000000..4ee7499 --- /dev/null +++ b/summer-student-scope.md @@ -0,0 +1,117 @@ +# Summer Student Project Scope + +This is the 3-month project brief. The detailed technical backlog is in `plan.md`; use this document to understand what is in scope for the summer and how to prioritize it. + +The goal is not to rewrite the CLI. The goal is to fix the highest-impact correctness issues, add development guardrails, and establish one clean pattern that future contributors can continue. + +## Project Goal + +Improve CBRAIN CLI reliability and maintainability by: + +- fixing the correctness bugs in Phase 1 of `plan.md`; +- adding tests, linting, formatting, and CI/pre-commit guardrails from Phase 2; +- improving output consistency for a selected command slice from Phase 3; +- starting, but not completing, the architecture cleanup in Phase 4; +- documenting what changed and what remains. + +Success means reviewed, tested, merged improvements. It does not mean touching every module. + +## Scope By Phase + +### Phase 1: Required + +Complete all Phase 1 items in `plan.md`. These are correctness fixes and should be treated as the baseline deliverable. + +### Phase 2: Mostly Required + +Complete the high-value guardrails from Phase 2 of `plan.md`: Ruff, tests, CI/pre-commit where practical, timeouts, credential permissions, versioning, and contributor checklist updates. + +### Phase 3: Selected Slice + +Do not try to make every command perfect. Pick a representative subset, preferably `project` and `task`, and make normal, JSON, and JSONL behavior consistent for that slice. + +Avoid broad public command renaming during the summer unless explicitly agreed. + +### Phase 4: Pattern Only + +Start Phase 4 by establishing a pattern, not by migrating the whole codebase. + +Recommended target: + +- define return contracts; +- add typed CLI exceptions; +- introduce a small `CbrainClient` or shared API helper; +- migrate one command family, preferably `task` or `project`. + +### Phase 5: Lightweight + +Document the architecture boundaries, test workflow, Ruff workflow, supported/tested CBRAIN API version if known, and remaining follow-up work. + +## Out Of Scope + +Do not attempt these unless the required work is already complete and reviewed: + +- migrating every command to `CbrainClient`; +- rewriting all return contracts across the whole CLI; +- making every command fully output-consistent; +- redesigning all destructive command safety behavior; +- broadly renaming public commands or options; +- replacing the capture test framework; +- adding external runtime dependencies beyond approved development tooling. + +## Suggested Timeline + +### Month 1: Correctness + +- Complete Phase 1. +- Add unit tests for those fixes. +- Keep capture tests passing. +- Start Ruff and CI/pre-commit work. + +### Month 2: Guardrails And Output + +- Complete most Phase 2 guardrails. +- Improve JSON/JSONL behavior for the selected command slice. +- Add regression tests for those output modes. +- Update contributor docs. + +### Month 3: Architecture Pattern + +- Introduce typed errors and/or a small API client. +- Migrate one command family to the new pattern. +- Add tests for the new pattern. +- Document the remaining backlog against `plan.md`. + +## Expectations For You + +- Do not rewrite the CLI. +- Fix correctness first, then add guardrails, then improve one architecture slice. +- Keep PRs small enough to review. +- Add tests for every behavior change. +- Preserve public command behavior unless a change is explicitly agreed. +- Do not change public command names without discussion. +- Do not add new direct `print()` calls in data modules. +- Keep tokens and credentials out of logs, fixtures, and PR descriptions. +- When in doubt, preserve existing behavior and document follow-up work. + +## PR Checklist + +Each PR should answer: + +- What user-visible behavior changed? +- What tests were added or updated? +- Do `ruff check .` and `ruff format --check .` pass? +- Were capture tests affected? +- Does JSON/JSONL behavior still make sense? +- Did any data module gain direct output behavior? +- Are credentials and tokens protected? + +## Definition Of Done + +The summer project is successful if: + +- all Phase 1 items in `plan.md` are fixed and tested; +- the project has regular lint/format/test guardrails; +- the selected command slice has consistent output behavior; +- one command family demonstrates the new architecture pattern; +- documentation explains what was done and what remains. From 8f160b28df2e1cd0c1b34bd253b409462412cab6 Mon Sep 17 00:00:00 2001 From: dlq Date: Wed, 13 May 2026 13:02:58 -0400 Subject: [PATCH 2/5] Document student fork PR workflow --- summer-student-scope.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/summer-student-scope.md b/summer-student-scope.md index 4ee7499..bad09e3 100644 --- a/summer-student-scope.md +++ b/summer-student-scope.md @@ -84,6 +84,8 @@ Do not attempt these unless the required work is already complete and reviewed: ## Expectations For You +- Work from your own fork of the repository. +- Open PRs from your fork to the upstream repository for review. - Do not rewrite the CLI. - Fix correctness first, then add guardrails, then improve one architecture slice. - Keep PRs small enough to review. @@ -98,6 +100,7 @@ Do not attempt these unless the required work is already complete and reviewed: Each PR should answer: +- Is this PR opened from your fork against the upstream repository? - What user-visible behavior changed? - What tests were added or updated? - Do `ruff check .` and `ruff format --check .` pass? From 1c12eb146c463651c33f4b0fda28d3d5bdfd0227 Mon Sep 17 00:00:00 2001 From: dlq Date: Wed, 13 May 2026 13:05:26 -0400 Subject: [PATCH 3/5] Clarify capture tests in student scope --- summer-student-scope.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/summer-student-scope.md b/summer-student-scope.md index bad09e3..17e2008 100644 --- a/summer-student-scope.md +++ b/summer-student-scope.md @@ -61,6 +61,8 @@ Do not attempt these unless the required work is already complete and reviewed: ## Suggested Timeline +Note: in this repository, "capture tests" means the existing shell-based CLI output regression tests in `capture_tests/`. They run CLI commands and compare captured terminal output against `expected_captures.txt`. + ### Month 1: Correctness - Complete Phase 1. From d076cb0d4ac5837372acbd89266c7e3579a7fa16 Mon Sep 17 00:00:00 2001 From: dlq Date: Wed, 13 May 2026 13:12:16 -0400 Subject: [PATCH 4/5] Add remote student onboarding notes --- summer-student-scope.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/summer-student-scope.md b/summer-student-scope.md index 17e2008..499f290 100644 --- a/summer-student-scope.md +++ b/summer-student-scope.md @@ -59,6 +59,26 @@ Do not attempt these unless the required work is already complete and reviewed: - replacing the capture test framework; - adding external runtime dependencies beyond approved development tooling. +## Repository Conventions To Know + +- The CLI is tested against CBRAIN, the server application at https://github.com/aces/cbrain. +- The capture tests expect a local CBRAIN test server on `localhost:3000` with the expected seeded test database. +- You are not expected to have production CBRAIN credentials. Use local/test credentials only. +- If setting up the CBRAIN test server blocks you, say so early. You can still write unit tests and note that capture tests were not run locally. +- `capture_tests/switch_session` writes test credentials directly to `$HOME/.config/cbrain/credentials.json`; do not run it casually against a real CLI session. +- The repo-root `cbrain` script is the local CLI entrypoint used by the capture tests. +- Runtime code is intended to use only the Python standard library unless a dependency is explicitly approved. +- Output formatting is part of the tested behavior. If command output changes, update capture expectations only when the new output is intentional. +- CBRAIN-specific terms such as `bourreau`, `data provider`, and `tool config` come from the server/domain model; refer to the server repository when behavior is unclear. + +## Remote Collaboration + +- Work asynchronously and make progress visible with small PRs or draft PRs. +- Mention your timezone and typical working hours in your first project update. +- Ask questions early when setup, CBRAIN terminology, or expected behavior is unclear. +- If you cannot run a test locally, say exactly which test was skipped and why. +- PR descriptions should include the commands you ran and any remaining uncertainty. + ## Suggested Timeline Note: in this repository, "capture tests" means the existing shell-based CLI output regression tests in `capture_tests/`. They run CLI commands and compare captured terminal output against `expected_captures.txt`. From b30408f49d0fe0a3f9146ec1b424b96e8525272e Mon Sep 17 00:00:00 2001 From: dlq Date: Wed, 13 May 2026 15:19:35 -0400 Subject: [PATCH 5/5] Document developer workflow --- README.md | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/README.md b/README.md index 9f02194..29ff433 100644 --- a/README.md +++ b/README.md @@ -145,6 +145,66 @@ This is part of [**a GSoC (Google Summer of Code) 2025** project](https://summer The lead developer is [axif0](https://github.com/axif0), mentored by the developers of the CBRAIN project. +### Developer Setup + +Install the CLI in editable mode with the development tools: + +```bash +python3 -m venv venv +source venv/bin/activate +pip install -e ".[dev]" +``` + +This project is a pure Python CLI. There is no separate compile or build step for normal development; installing in editable mode is enough to run the local `cbrain` command. + +### Linting And Formatting + +Ruff is used for linting and formatting: + +```bash +ruff check . +ruff format . +``` + +To check formatting without changing files: + +```bash +ruff format --check . +``` + +### Pre-Commit Hooks + +The repository includes a `.pre-commit-config.yaml`. Install the hooks with: + +```bash +pre-commit install +``` + +The hooks currently: + +- trim trailing whitespace; +- ensure files end with a newline; +- check YAML syntax; +- check Markdown links; +- run `ruff --fix`; +- run `ruff format`. + +The generated capture fixture `capture_tests/expected_captures.txt` is excluded from whitespace and Ruff hooks because exact captured output is intentional there. + +To run the hooks manually: + +```bash +pre-commit run --all-files +``` + +### Tests + +The existing test suite is based on capture tests in `capture_tests/`. These are shell-based CLI output regression tests: they run commands from `capture_tests/cbrain_cli_commands` and compare the captured terminal output against `capture_tests/expected_captures.txt`. + +Capture tests require a local CBRAIN test server on `localhost:3000` with the expected test database seed. The GitHub Actions workflow sets this up by checking out the CBRAIN server repository at https://github.com/aces/cbrain. + +When command output intentionally changes, update `capture_tests/expected_captures.txt`. When behavior changes without intended output changes, prefer adding focused unit tests where possible. + ### Continuous Integration Continuous Integration (CI) tests and framework were initially configured by P. Rioux, providing automated validation of the codebase. This infrastructure follows best open source practices and ensures code quality through automated testing.