fix(review): drop the cubic default flags that produced a false clean review - #529
Merged
Merged
Conversation
… review `.please/config.yml` pinned `default-flags: "--json -b"` for every local cubic invocation. `-b, --base` takes a string, so a bare `-b` swallows the token that follows it; cubic then fails internally to produce a diff but still exits 0 with an empty `issues` array. To every caller that is indistinguishable from a review that ran and found nothing, so the misconfiguration surfaced as a clean review rather than as a broken reviewer. Removing the key rather than repairing it, because the reviewer agent already does better without one: it passes `-j` itself and selects the scope from the working tree — uncommitted changes when dirty, the base branch when clean. A configured value overrode that adaptive choice on every run, including `/review:apply-review` Step 5, which runs before the fix commit and wants exactly the uncommitted-changes scope. Verified `setup-env.sh --print` now exports only `REVIEW_CUBIC_ENABLED='true'`, where before it also exported `REVIEW_CUBIC_DEFAULT_FLAGS='--json -b'`. Only the locally-invoked CLI path was affected; the cubic GitHub app reviews pushed commits normally and was never impacted. Closes #514
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the 'default-flags' configuration for the 'cubic' reviewer in '.please/config.yml' and adds a detailed comment explaining the change. The previous configuration of '--json -b' caused silent failures and false clean reviews because the bare '-b' flag swallowed the subsequent token. I have no further feedback to provide as there are no review comments.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c95e37464
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The comment claimed the bare `-b` "swallowed the following token". In the invocation that actually failed — `cubic review -j --json -b`, the agent's `-j` plus the configured words — `-b` sat last, so there was no following token to swallow. It is a string option left without a value, which is why cubic could not produce a diff. The swallow-the-next-token form is a real hazard for this flag, but it belongs to a different invocation (the reviewer agent's own `cubic review -b -j` fallback, upstream), not to the one this repository's config produced. #514 stated it correctly as "with nothing after it the flag consumes the next token — here, nothing at all"; the précis dropped the qualifier. Found by the silent-failure finder while reviewing the parent commit.
Three reviewers landed on the same gap: the rationale comment described the agent's adaptive scope selection as if both of its branches worked, when this change repairs only one of them. A maintainer reading the config would have inferred that removing the key closed every false-clean path. The comment now says the dirty-tree path is the one repaired, and that the clean-tree branch issues `cubic review -b -j` — `-b` again with no base ref of its own — so a local review of a clean worktree can still come back falsely clean. That command belongs to the review plugin, and no key in this file reaches it, so #514 carries it as the upstream follow-up. The first paragraph is reworded along with it: it claimed the agent "picks the base branch when it is clean", which the new paragraph then contradicted. It now says the agent *attempts* a base-branch review, and the second paragraph gives both branches the same root cause. One deliberate difference from the reviewers' wording. Two of them stated that the fallback's `-b` consumes the following `-j` as its base argument; that is not asserted here, because confirming what yargs does with a value token beginning with `-` would take running the parser, and an unverified swallow claim in this same PR already had to be corrected. "No base ref of its own" is true without that check and carries the same conclusion. Applied from PR #529 review: greptile-apps, chatgpt-codex-connector, and the gpt finder (which caught the self-contradiction the first two fixes created).
Contributor
|
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.
Closes #514.
The defect
.please/config.ymlpinneddefault-flags: "--json -b"for every local cubic invocation.-b, --baseis declared[string]incubic review --help. The reviewer agent buildscubic review -j <configured words>, so the configured value put-blast on the commandline, with no value after it. cubic then fails internally to produce a diff — but still
exits
0with an emptyissuesarray:To every caller that is indistinguishable from a review that ran and genuinely found
nothing. The misconfiguration did not surface as a broken reviewer; it surfaced as a
clean review.
Why remove the key rather than repair it
The reviewer agent already does better without one. Its execution rules:
cubic review -j <configured words>cubic review -jcubic review -b -jSo it passes
-jitself — making the configured--jsonredundant — and it selects thescope from the working tree. A configured value overrides that adaptive choice on every
run, including
/review:apply-reviewStep 5, which runs before the fix commit and wantsexactly the uncommitted-changes scope. Removing the key restores both behaviors.
Verification
setup-env.sh --print, the only consumer of this key:enabled: trueis preserved; only the flag export is gone.Scope
Only the locally-invoked CLI path was affected. The cubic GitHub app reviews pushed
commits normally and was never impacted — it posted real findings throughout #508, #526,
and #528.
Not fixed here — upstream,
passionfactory/reviewTwo related hazards live in the review plugin, not in this repository, so this PR does not
touch them:
cubic review -b -j— the same bare-b, whichhere swallows
-j.which is what let this misconfiguration stay invisible. cubic default-flags "--json -b" silently returns a false-clean review #514 raised this as "worth
checking alongside".
Docs
No doc surface describes this key. The root READMEs mention cubic only as a listed review
service, and
.please/config.ymlis workspace tooling rather than a shunt config key,endpoint, CLI, or provider surface. The rationale is recorded as an inline comment at the
removal site instead.
Summary by cubic
Removes the
cubicdefault-flagsfrom.please/config.yml, which previously forced--json -bon every local run.-btakes a string and was left without a value, so cubic failed internally to produce a diff but still exited0with an empty issues array — a misconfigured reviewer looked like a clean review. Now the agent passes-jitself and selects scope from the working tree, so dirty-tree reviews reflect actual changes. Closes #514.Scope
cubic review -b -jlives in the review plugin upstream, where bare-bcan still yield a false clean review, and no key in this file reaches it.Written for commit 755af41. Summary will update on new commits.