Skip to content

Implement Remaining plan.md Tasks (16, 18, 19 & 21) - #51

Open
RafsanNeloy wants to merge 3 commits into
aces:mainfrom
RafsanNeloy:remaining_phaseworks
Open

Implement Remaining plan.md Tasks (16, 18, 19 & 21)#51
RafsanNeloy wants to merge 3 commits into
aces:mainfrom
RafsanNeloy:remaining_phaseworks

Conversation

@RafsanNeloy

Copy link
Copy Markdown
Contributor

Contributor checklist

  • This pull request is on a separate branch and not the main branch
  • I ran pre-commit run --all-files and/or pytest as appropriate (see Tests in the README). A clean Ruff/pre-commit run is necessary but not sufficient—behavior still needs tests and review.
  • If CLI output changed intentionally, I updated capture_tests/expected_captures.txt
  • If command behavior changed, I checked normal, --json, and --jsonl output modes (or noted why not applicable)
  • Data-layer modules do not add direct print() calls for user-visible output
  • Public command names, flags, and default behavior remain compatible unless this PR explicitly documents a breaking change
  • No credentials, tokens, or session data appear in code, fixtures, logs, or this PR description

Type of change

  • Bug fix
  • New feature or command behavior
  • Documentation
  • Tests only
  • Other (describe below)

Description

Added new task management operations, refined project switching, strengthened error handling and debugging, modernized CLI argument parsing, and updated tests.


Test plan

$ ./cbrain --verbose file list

[DEBUG] GET /userfiles?page=1&per_page=25
[DEBUG] → HTTP 200
ID Type           File Name                       
-- -------------- --------------------------------
4  TextFile       cbrain_dispatch.txt
5  TextFile       cbrain_dispatchh.txt
6  CbrainFileList file_list.24233.1778257539.cbcsv

RafsanNeloy and others added 3 commits August 6, 2026 00:30
Signed-off-by: rafsanneloy <rafsanneloy@gmail.com>
Signed-off-by: rafsanneloy <rafsanneloy@gmail.com>
Signed-off-by: Rafsanul Islam Neloy <89649374+RafsanNeloy@users.noreply.github.com>
@RafsanNeloy

Copy link
Copy Markdown
Contributor Author

Previously-

$./cbrain <cli-cmds>
Authentication error (401): Unauthorized
Error: Access denied. Please log in using authorized credentials.
$ ./cbrain login
Already logged in. Use 'cbrain logout' to logout.

Now we should treat "logged in" as server accept token, not file exist -

$./cbrain <cli-cmds>
Authentication error (401): Unauthorized
Error: Session expired or invalid. Run 'cbrain logout' then 'cbrain login'.
$./cbrain login
Saved session expired. Please log in again.
Enter CBRAIN server base URL [default: localhost:3000]:

login > file? > GET /session OK? > stop ("already in")
login > file? > GET 401 > prompt URL/user/pass > save new token

task/… > use token > 401 > say expired + logout/login hint

@dlq

dlq commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Good work! This closes out four backlog items cleanly and the structure is right.
I checked it out locally: pytest (309 passed), ruff check/ruff format --check clean,
and it trial-merges into main with no conflicts and tests still green. I also drove the
new task operation against a local stub server and confirmed the request body, the
normal/--json/--jsonl output, and the validation errors all behave as intended.

Two things I especially liked: operation_task() now returns data with the handler and
formatter owning presentation, which is exactly what items 19 and 21 asked for; and the
backward-compatible aliases (dataprovider, --dp-id) mean nobody's existing scripts
break. The --debug output is also credential-safe — tokens only ever travel in headers,
and only the method and path are logged.

A few things to tidy up:

  1. Test plan vs. implementation. The description shows ./cbrain --verbose file list,
    but the flag you implemented is --debug and --verbose is rejected by the parser. Worth
    updating the description to the command you actually ran (or adding --verbose as an alias
    if you'd prefer both — item 18 allows either name).

  2. Operation names use underscores. archive_file, zap_wd, restart_cluster etc. sit a
    little oddly next to the rest of the public vocabulary, which is dashed (is-alive,
    delete-unregistered-files, and --filter bourreau-id, which even has a dash→underscore
    translator). Since item 16 is about settling exactly this, it'd be good to accept the dashed
    spellings too and translate them the same way — the underscore forms can keep working.

  3. --debug is only tested at the parser level. test_global_debug_flag asserts
    args.debug is True, which is a good start, but nothing checks that the [DEBUG] lines
    actually reach stderr, that they stay off by default, or that no token appears in them. A
    small capsys test would lock in the behaviour that item 18 is really about. One caution:
    _debug is module-global, so such a test should reset it in a fixture or it'll leak into
    other tests.

  4. Minor: in _request, the debug display splits the target on ? and only re-appends the
    query when params is passed as a dict, so POST /groups/switch?id=2 logs as
    POST /groups/switch. Purely cosmetic, but the id is the useful part.

  5. Optional follow-up on the session probe. The expired-token detection is a genuinely
    good addition, and it's scope you took on beyond the four backlog items. One loose end:
    the URLError branch routes an unreachable server to the same "Already logged in" message.
    That's not a regression — it printed that before your change too, since there was no probe
    at all — but now that the code does reach the network, a third branch could report a
    connectivity failure as one. Fine to leave for a separate PR.

None of these are structural — the design decisions here are sound. Happy to re-review once
they're addressed.

@dlq

dlq commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

One more note, since that was a long list for a PR I'm happy with: none of it is
blocking in the sense of "this is broken." To merge, I only need (1) — the description
updated to match --debug. (3) and (4) are small and would be nice in this PR; (5) is
explicitly a separate one.

On (2), don't spend time deciding — that's my call as maintainer, and I'll follow up with
which spelling we standardise on. If it's dashed, it's a small translator in the choices
list, same pattern as --filter bourreau-id already uses.

The design decisions in this PR are the hard part, and you got those right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants