fix(cli): make flux ready respect the default project - #87
Conversation
`flux ready` was the only command that ignored the default project from `.flux/config.json`. The dispatcher already computes `defaultProject` and passes it to `projectCommand` and `taskCommand`, but the `readyCommand` call was missing the argument, so with no project on the command line `getReadyTasks` received `undefined` and returned unblocked tasks from every project on the board. In a multi-project board that silently mixes other projects into the answer, and since `ready` sorts by priority, a P0 belonging to another project ends up at the top of the list. Explicit selection still wins: a positional project or `-p`/`--project` takes precedence over the default. The new `--all` flag asks for the whole board on purpose, which is what bare `flux ready` used to do. Also documents the positional project argument in `--help`, which was missing even though the argument already worked.
thebulklord
left a comment
There was a problem hiding this comment.
This PR fixes a real inconsistency where flux ready was the only command ignoring the default project from .flux/config.json. The fix is minimal and surgical: thread defaultProject into readyCommand (same pattern taskCommand and projectCommand already use), add a --all escape hatch for the old cross-project behavior, and update help/docs. The precedence chain (--all > explicit project > default > undefined) is clean and the three new tests nail the key paths. The intentional behavior change for bare flux ready is clearly called out in the PR description, which is the right call — making the common case correct beats preserving a silent footgun.
Strengths
- The fix is one line of logic in
ready.tsplus one argument at the call site. No refactoring, no new abstractions. Exactly the right scope for a bug fix. - Precedence is unambiguous:
--allshort-circuits toundefined, then explicit project (positional or-p/--project) wins over the stored default. This mirrorstaskCommand's existing pattern, so the mental model is consistent across the CLI. - The three new tests cover the fallback, the explicit-over-default override, and the
--allopt-out. Existing tests that callreadyCommandwithout the fourth argument still pass becausedefaultProjectdefaults toundefined, preserving the old behavior for repos without a configured project. - Help text and
docs/cli.mdare updated in the same commit, so the new--allflag and positional argument aren't a hidden feature. - The PR description is honest about the breaking change and offers to reshape the default if maintainers prefer. That's good citizenship.
Suggestions
- Consider adding a test for
flux ready proj-1 --allto lock in the current behavior where--allwins over an explicit project. Right now the code makes--alla hard override, which is defensible, but a test documents the intent so a future refactor doesn't accidentally flip it. - The
--allflag isn't listed in the flags reference table at the bottom ofdocs/cli.md. It's covered in the quick-commands block, but the table is where people look for flag semantics. A one-line addition would close the gap. - Minor: the
--helpline now shows[project]in yellow (positional) and[--all]in green (flag), which is consistent with the rest of the help output. Nice.
Risk and coverage
- Verified the logic in
ready.tsby tracing all four input combinations (no project, explicit project, default project,--all) against thegetReadyTaskscall. All paths produce the expectedprojectIdvalue. - Verified the dispatcher in
index.tspassesdefaultProject(sourced frominitStorage().project) toreadyCommand, matching the pattern used fortaskCommandandprojectCommandtwo lines above. - Verified the three new tests exercise the correct argument positions and assert the right
getReadyTaskscall. The seven pre-existing tests are untouched and their calls omit the fourth argument, so they exercise thedefaultProject === undefinedpath. - Did not execute the test suite or build; the PR description reports 274/274 green. No static analysis commands were configured in the repository analysis.
- No security concerns:
defaultProjectoriginates from a local config file read byinitStorage(), the same trust boundary as every other command. No new external input, no injection surface.
Findings:
- [nit] packages/cli/tests/ready.test.ts:78 - Add a test for --all combined with an explicit project: The current logic makes
--alla hard override:flags.all === true ? undefined : explicitProject || defaultProject. Soflux ready proj-1 --allreturns all projects, ignoringproj-1. That's a reasonable design choice, but there's no test pinning it down. A future refactor that reorders the ternary would silently change behavior with no test failure. Suggest adding:
it('--all wins over an explicit project', async () => {
mockGetReadyTasks.mockResolvedValue([]);
await readyCommand(['proj-1'], { all: true }, false, 'proj-default');
expect(mockGetReadyTasks).toHaveBeenCalledWith(undefined);
});Not blocking — the current behavior is correct — but it's a one-liner that prevents a future regression.
- [nit] docs/cli.md:148 - Add --all to the flags reference table: The flags table at the bottom of
docs/cli.mdlists--json,-P/--priority,-e/--epic,--note, and--status, but not--all. The quick-commands block above covers it, but the table is the canonical reference for flag semantics. A row like| --all | Show ready tasks across all projects (overrides default project) |would close the gap. Non-blocking since the quick-commands section already documents it.
Coverage notes:
-
Traced all four projectId resolution paths in ready.ts (no project, explicit, default, --all) against the getReadyTasks call — all correct.
-
Verified the dispatcher in index.ts passes defaultProject from initStorage().project, consistent with taskCommand and projectCommand.
-
Verified the three new tests assert the correct getReadyTasks arguments and that the seven pre-existing tests omit the fourth parameter (defaultProject = undefined), preserving old behavior.
-
Did not execute the test suite or build; PR description reports 274/274 green. No static analysis or SAST commands were configured.
-
No secrets detected in changed files. No new external input or trust-boundary changes.
-
Caller/reference search was skipped (rg not available), but the only caller of readyCommand is the dispatcher in index.ts, which is in the diff.
Review checklist:
| Area | Status | Area | Status |
|---|---|---|---|
| Intent match | ✅ reviewed against PR text/commits | Correctness | |
| Security | ✅ built-in secret scan | Regression risk | ✅ checkout context, 4 file(s), 1 reference set(s) |
| Tests | ➖ reviewed from code/test context | Static checks | ➖ not configured |
| Maintainability | Performance |
✅ Clean fix, bro. You spotted the one command that was skipping leg day while all the others were hitting the default project, and you fixed it with the minimum effective dose. The --all escape hatch is the right call — nobody should have to guess that bare flux ready used to mean the whole board. Ship it, then go hit a double-bicep set to celebrate.
|
|
||
| await readyCommand([], { all: true }, false, 'proj-default'); | ||
|
|
||
| expect(mockGetReadyTasks).toHaveBeenCalledWith(undefined); |
There was a problem hiding this comment.
[nit] Add a test for --all combined with an explicit project
The current logic makes --all a hard override: flags.all === true ? undefined : explicitProject || defaultProject. So flux ready proj-1 --all returns all projects, ignoring proj-1. That's a reasonable design choice, but there's no test pinning it down. A future refactor that reorders the ternary would silently change behavior with no test failure. Suggest adding:
it('--all wins over an explicit project', async () => {
mockGetReadyTasks.mockResolvedValue([]);
await readyCommand(['proj-1'], { all: true }, false, 'proj-default');
expect(mockGetReadyTasks).toHaveBeenCalledWith(undefined);
});Not blocking — the current behavior is correct — but it's a one-liner that prevents a future regression.
Review follow-up. `--all` was documented in the quick commands block and in the `ready` usage line, but missing from both flag lists: the `Flags:` section of `flux --help` and the flags table in docs/cli.md. Adds the test the review suggested, `--all` together with an explicit project, which pins down that `--all` is a hard override rather than a fallback.
|
Thanks for the review. Both nits are now addressed, plus a third instance of the same omission that the review did not catch.
Your suggested test is in as well: On the colouring of The workflows are still sitting at The open question from the PR body still stands and is for @sirsjg rather than a follow-up commit: whether making bare |
|
Good catch — this was an oversight on my part, not a design choice. |
The problem
flux readyis the only command that ignores the default project stored in.flux/config.json. In a repository whose default project is set withflux project use,flux task listandflux primescope their answer to that project, butflux readyreturns unblocked tasks from every project on the board.The cause is a missing argument rather than a design decision. In
packages/cli/src/index.tsthe dispatcher computesdefaultProjectfrominitStorage()and hands it toprojectCommandandtaskCommand, but thereadyCommandcall a few lines below leaves it out, even though the variable is in scope.readyCommandthen resolves the project fromargs[0],-pand--projectonly, andgetReadyTasks(undefined)skips the project filter entirely.This is easy to miss with a single project on the board, because the unfiltered answer happens to match. It starts hurting as soon as there is a second one, and it hurts in a particular way:
readysorts by priority, so a P0 that belongs to an unrelated project lands at the top of the list.--helpdoes not mention thatreadyaccepts a project at all, which is what keeps the whole thing out of sight.Reproducing it takes two projects and a repository pointed at one of them:
The change
readyCommandnow takesdefaultProjectas its last parameter and uses it when nothing more specific was given, which is the same shapetaskCommandalready uses forflux task list. Precedence is unchanged where it was already defined: a positional project or-p/--projectstill wins over the default.Since bare
flux readyused to mean "the whole board", and that is a reasonable thing to want, this adds--allto ask for it explicitly. Worth calling out plainly: for anyone relying on bareflux readyas a cross-project view, the default output changes and--allis the replacement. It seemed better to make the common case correct and give the global view a name than to leave the default wrong, but that trade-off is yours to make, and I am happy to reshape this if you would rather keep the old default and require the project explicitly.The
--helpline forreadynow shows the positional argument and--all, anddocs/cli.mdgets the three variants.Verification
bun run buildpasses and the full suite is green: 274 tests across 18 files, no failures. Three cases were added topackages/cli/tests/ready.test.ts, covering the fallback to the default project, explicit selection winning over it, and--allignoring it. The seven existingreadytests were left untouched and still pass, since they callreadyCommandwithout the new parameter.Checked against a real three-project board as well, from a repository whose default project is the first one: before the change bare
flux readyreturned 36 tasks spread over the three projects; after it, 17 tasks from that project alone,--allreturns the same 36, and passing another project's id returns its 15.