Repository navigation
cli: validate --thinking values against THINKING_LEVELS - #496
DawnofGenX wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly and concisely describes the main change: validation of CLI --thinking values against THINKING_LEVELS. |
| Description check | |
The description includes all required sections, explains the problem and approach, links the fixing issue, documents tests and results, and records checklist and AI-assistance details. The reported va… |
| Linked Issues check | |
Issue [#421] has coding requirements, and the reviewed head satisfies them. packages/cli/src/main.ts imports THINKING_LEVELS from @axl/protocol and validates --thinking before assigning the va… |
| Out of Scope Changes check | |
The change summary identifies only packages/cli/src/main.ts and packages/cli/test/unsafe-cli.test.ts. The source change implements [#421] validation, and the test verifies that behavior. No unrela… |
| Docstring Coverage | |
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. |
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔇 Additional comments: 2
packages/cli/test/unsafe-cli.test.ts: 1
128-135: LGTM!packages/cli/src/main.ts: 1
241-248: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.Replace the
as ThinkingLevelassertions with a type guard.The code uses
level as ThinkingLeveltwice. The first use is only there to satisfyincludeson areadonly ThinkingLevel[]. The second use trusts the earlier check. Both assertions bypass the compiler. Use one type guard so the compiler narrowslevel.The message builder also has a one-element branch.
THINKING_LEVELShas seven entries, so that branch never runs. Keep the list formatting simple.Proposed fix
- if (!THINKING_LEVELS.includes(level as ThinkingLevel)) { - const expected = - THINKING_LEVELS.length > 1 - ? `${THINKING_LEVELS.slice(0, -1).join(", ")}, or ${THINKING_LEVELS[THINKING_LEVELS.length - 1]}` - : THINKING_LEVELS[0]; - throw new Error(`Unknown thinking level ${level}; expected ${expected}`); - } - parsed.thinking = level as ThinkingLevel; + const known = THINKING_LEVELS.find((candidate) => candidate === level); + if (known === undefined) { + const expected = `${THINKING_LEVELS.slice(0, -1).join(", ")}, or ${THINKING_LEVELS.at(-1)}`; + throw new Error(`Unknown thinking level ${level}; expected ${expected}`); + } + parsed.thinking = known;Based on learnings: in TypeScript, prefer type narrowing over
asassertions and use runtime checks instead.Source: Learnings
ℹ️ Review info
⚙️ Run configuration
- Configuration used: defaults
- Review profile: CHILL
- Plan: Advanced
- Run ID:
5c2e7488-3c96-4e80-b0ef-462452e3304b
📒 Files selected for processing: 2
packages/cli/src/main.tspackages/cli/test/unsafe-cli.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
Check the value against THINKING_LEVELS from @axl/protocol and throw an error in the same style as --profile when an invalid level is provided. Add a test for the invalid case. Fixes Observal#421 Signed-off-by: Priyansh <pkansara0412@gmail.com>
77cafc4 to
a1b73f3
Compare
1 participant
Purpose
axl --thinking <level>accepted any string. The value was cast straight toThinkingLevelwithout a check, so a typo likeaxl --thinking hgihwas not caught while arguments were parsed. Other options with a fixed value set, such as--profileand--sandbox, already fail fast with a clear message.Fixes
Fixes #421
Approach
In
parseArguments(packages/cli/src/main.ts), check the--thinkingvalue againstTHINKING_LEVELSfrom@axl/protocol. On a bad value, throw in the same style as--profile:Unknown thinking level hgih; expected off, minimal, low, medium, high, xhigh, or max. No compatibility or security impact; the check runs before any credential or daemon work.How was this tested?
pnpm --filter @axl/cli test— 55 pass, 0 fail (3 skipped).rejects invalid thinking level argumentsassertsaxl --thinking hgihexits non-zero with the expected message.main.tschange makes the new test fail (1 fail); restoring it makes it pass.pnpm --filter @axl/cli typecheckclean.Learning
The
--profilevalidation was the existing pattern to mirror, so the error message and throw site stay consistent.Checklist
REUSE.toml.Signed-off-bytrailer.AI assistance
Summary by CodeRabbit
--thinkingvalues and reports the accepted levels.