fix: align create/update flag surface across issue, issue-template, action, calendar (HULY-8) - #43
Conversation
|
Warning Review limit reached
Next review available in: 36 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe CLI adds body, body-file, kind, due-date, label, and attachment-related update options. Issue and template descriptions support file input and precedence rules. Event updates accept Markdown bodies. Task reparenting requests now return validation guidance. ChangesCLI update options
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/resources/issue-template.ts`:
- Around line 229-233: Update the body-selection logic in the issue template
flow so the opts.body branch checks for undefined rather than truthiness,
preserving an explicitly provided empty string and its precedence over
opts.description. Keep bodyFile handling and the description fallback unchanged.
In `@packages/cli/src/resources/issue.ts`:
- Around line 1157-1167: Move the `updateMarkup` call in the issue update flow
so body input is resolved and all `--kind`/`--due` validation completes before
any write. In the surrounding update command, return on `opts.dryRun` before
invoking `updateMarkup` or other mutations, while preserving the existing body
handling and validation behavior.
In `@packages/cli/src/resources/todo.ts`:
- Around line 567-581: Move the unsupported reparenting validation for
opts.attachedTo and opts.attachedToClass before the connectCli call in the
update flow. Keep the existing CliError, exit code, and delete-and-recreate
guidance unchanged, and remove the later duplicate check so invalid reparenting
requests fail before workspace or platform setup.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f69b9181-f1ad-470e-bae3-9b56c7910ddb
📒 Files selected for processing (5)
packages/cli/src/cli.tspackages/cli/src/resources/calendar.tspackages/cli/src/resources/issue-template.tspackages/cli/src/resources/issue.tspackages/cli/src/resources/todo.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Kilo Code Review
🧰 Additional context used
🪛 ast-grep (0.45.0)
packages/cli/src/resources/issue-template.ts
[warning] 230-230: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(opts.bodyFile, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🪛 GitHub Actions: CI / 0_Build, format, lint, typecheck, test.txt
packages/cli/src/cli.ts
[error] 1-1: oxfmt formatting check failed. Run 'oxfmt' without '--check' to fix formatting issues.
🪛 GitHub Actions: CI / Build, format, lint, typecheck, test
packages/cli/src/cli.ts
[error] 1-1: oxfmt formatting check failed. Run 'pnpm format:check' without '--check' or apply the formatter to fix this file.
🔇 Additional comments (5)
packages/cli/src/resources/todo.ts (1)
555-556: LGTM!packages/cli/src/resources/issue.ts (1)
1083-1088: LGTM!Also applies to: 1180-1196, 1206-1206
packages/cli/src/cli.ts (1)
966-988: LGTM!Also applies to: 1337-1337, 2140-2156, 2572-2572
packages/cli/src/resources/issue-template.ts (1)
207-207: LGTM!Also applies to: 234-239
packages/cli/src/resources/calendar.ts (1)
701-701: LGTM!Also applies to: 724-727, 744-744
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| .addHelpText( | ||
| 'after', | ||
| ` | ||
| Note on --attached-to / --attached-to-class: |
There was a problem hiding this comment.
WARNING: Help text is misleading — it claims reparenting is supported, then immediately says it isn't exposed.
The first sentence ("Reparenting is supported via 'removeCollection' on the old parent + 'addCollection' on the new parent…") reads like a feature description, but the CLI just throws "reparenting an action via update is not yet supported" whenever these flags are passed. The WorkSlot preservation and cascade semantics are implementation details the user can't act on. Consider rewriting to lead with the rejection: e.g., "Reparenting is not supported. To move an action, delete it and recreate it under the new parent. (The new action will receive a new _id; downstream references will break.)"
| .option('--task-type <name|id>') | ||
| .option('--kind <ref>', 'TaskType ref (e.g. tracker:taskTypes:Issue); power-user bypass of name lookup') | ||
| .option('--due <iso>', 'ISO 8601 e.g. 2026-07-01T14:00:00Z') | ||
| .option('--label <l...>', 'repeatable: --label bug --label auth (replaces existing labels)') |
There was a problem hiding this comment.
SUGGESTION: --label <l...> requires at least one value, so users cannot clear all labels via this flag — they have to know to use --set labels=null or --unset labels. The help text mentions the additive subcommands but doesn't mention how to clear all labels. Either document the limitation in the option description ("use --unset labels to clear") or add support for --label "" (or an explicit --clear-labels) to set ops.labels = [].
| } | ||
| // HULY-8: --label replaces the labels array, matching issue create. | ||
| // Use 'issue label add' / 'issue label remove' for additive/subtractive | ||
| // semantics — those go through the TagReference collection, not this |
There was a problem hiding this comment.
WARNING: --label "" (single empty string) passes through as opts.label = [''] and this branch fires, so ops.labels = [''] is sent to the server — silently creating an empty-string label on the issue. Add a guard that rejects empty/whitespace-only label values, e.g. const cleaned = opts.label?.filter((l) => l.trim().length > 0); if (cleaned && cleaned.length > 0) ops.labels = cleaned.
| // Use 'issue label add' / 'issue label remove' for additive/subtractive | ||
| // semantics — those go through the TagReference collection, not this | ||
| // direct field set. | ||
| if (opts.label !== undefined) ops.labels = opts.label |
There was a problem hiding this comment.
SUGGESTION: No validation that the supplied labels exist in the workspace. A typo like --label bugx will silently set the issue's labels to a non-existent tag, mirroring the create-time gap (so it's consistent) but worth flagging — consider resolving label names against the workspace's TagReference set and throwing on unknown values, or at least warning.
| const ops: Record<string, unknown> = {} | ||
| if (opts.title) ops.title = opts.title | ||
| if (opts.description !== undefined) ops.description = opts.description | ||
| // HULY-8: --body takes precedence over --description, mirroring createEvent |
There was a problem hiding this comment.
SUGGESTION: ops.description = opts.body writes raw markup text directly to the event, which is exactly the bug pattern called out in HULY-20 (per the PR description, comment add/update does the same and renders literal <h1>/<p> tags). The issue update path in this PR correctly routes through updateMarkup (packages/cli/src/resources/issue.ts:1157) so the description field gets a MarkupBlobRef. Calendar events should follow the same pattern — route --body and --description through uploadMarkup/updateMarkup so rich content renders properly in the event UI.
| if (opts.body) ops.description = opts.body | ||
| // HULY-8: --body-file mirrors the file-read path used by createIssueTemplate | ||
| // (lines 165-167). --body still wins if both are passed (matches create). | ||
| if (opts.bodyFile) { |
There was a problem hiding this comment.
SUGGESTION: Same HULY-20 bug pattern — ops.description = (await fs.readFile(opts.bodyFile, 'utf8')).trim() writes raw markup text directly. The pre-existing --body path on this same function had the same bug, so the new --body-file path is at least consistent. But neither is correct: the issue-template's description field expects a MarkupBlobRef, so both --body and --body-file on update should call uploadMarkup (or updateMarkup) and store the returned ref, mirroring how updateIssue handles --body.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Both previously reported SUGGESTIONs from commit 3598506 are resolved by this incremental commit (91231dc):
No new issues introduced in the incremental diff (2 changed files, ~12 changed lines). Files Reviewed (2 files)
Previous Review Summaries (2 snapshots, latest commit 3598506)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 3598506)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Previously Reported Issues (re-verified against 3598506)
Files Reviewed (5 files)
Fix these issues in Kilo Cloud Previous review (commit 11f4be5)Status: 6 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (5 files)
Reviewed by minimax-m3 · Input: 32.4K · Output: 3.8K · Cached: 157.2K |
11f4be5 to
3598506
Compare
Review feedback addressed (amended into commit 3598506)CI now passes (was failing on CodeRabbit inline comments
Kilo Code Review warnings + suggestions
Verification
Branch force-updated; ready for re-review. |
| // after the dry-run guard below — otherwise `--body ... --dry-run` would | ||
| // persist. The --kind / --due resolve calls are intentionally kept where | ||
| // they are (matches the existing resolveStatus / resolvePriority pattern). | ||
| const body = await readBody(opts) |
There was a problem hiding this comment.
SUGGESTION: --body "" is silently a no-op on issue update — asymmetric with the issue-template update fix in this PR.
readBody (issue.ts:91-101) uses truthy checks (if (opts.body) return opts.body), so --body "" makes readBody return undefined. Then markupBody = body ?? opts.description resolves to opts.description (also undefined unless the user passed --description), markupRequested = false, and updateMarkup is never called.
Meanwhile, on huly issue-template update (issue-template.ts:235 after this PR's CodeRabbit-driven fix), else if (opts.body !== undefined) ops.description = opts.body correctly clears the description. The two update commands now have inconsistent --body "" behavior, even though both got --body/--body-file parity in the same PR.
Either mirror the fix by changing readBody's if (opts.body) to if (opts.body !== undefined) (one-line change, also fixes the same latent issue on createIssue), or document the asymmetry. The downstream updateMarkup already handles empty body by sending EMPTY_PROSEMIRROR_DOC, so the fix is safe.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| .option('--body-file <path>') | ||
| .option('--task-type <name|id>') | ||
| .option('--kind <ref>', 'TaskType ref (e.g. tracker:taskTypes:Issue); power-user bypass of name lookup') | ||
| .option('--due <iso>', 'ISO 8601 e.g. 2026-07-01T14:00:00Z') |
There was a problem hiding this comment.
SUGGESTION: --due <iso> help text doesn't mention that empty string clears the existing due date — the only way to clear dueDate on update.
The implementation at issue.ts:1170 explicitly handles this case (opts.due === '' ? null : parseDate(opts.due, '--due')), but the option description only documents the ISO format. Users have no way to discover the clear-via-empty-string convention. Mirror the documentation pattern used for --label on line 973 ("use --unset labels to clear all"):
| .option('--due <iso>', 'ISO 8601 e.g. 2026-07-01T14:00:00Z') | |
| .option('--due <iso>', "ISO 8601 e.g. 2026-07-01T14:00:00Z; pass --due '' to clear the existing due date") |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
…ction, calendar (HULY-8)
`huly issue update` was missing several flags that `huly issue create` accepts
(`--label`, `--kind`, `--body`, `--body-file`, `--due`). The same pattern
appeared in three other write-asymmetric surfaces:
- `huly issue-template update` was missing `--body-file`
- `huly action update` was missing `--attached-to` / `--attached-to-class`
- `huly calendar update` was missing `--body`
Each fix mirrors the corresponding `create` handler:
* `--label` on issue update: replaces the labels array, matching create
* `--kind` on issue update: power-user TaskType ref, skipping name lookup
* `--body` / `--body-file` on issue update: uploadMarkup path, body
takes precedence over --description (mirrors createIssue descriptionSource)
* `--due` on issue update: empty string clears, ISO string sets
* `--body-file` on issue-template update: file-read path matching create
* `--attached-to` / `--attached-to-class` on action update: wired but
rejected with a clear 'delete + recreate' workaround, since true
reparenting needs removeCollection + addCollection + WorkSlot migration
(out of scope; a dedicated 'action move' subcommand is the right fix)
* `--body` on calendar update: body wins over --description, matching
createEvent
Also tidies the 'nothing to update' error hints and help text to enumerate
all accepted flags, removing the stale `--kind` reference in the issue
update hint that advertised a flag the CLI never registered.
Verified on http://localhost:7180 with credentials test@aaravlabs.com/test
against the `huly_v7_test` compose stack.
3598506 to
91231dc
Compare
Two new review issues addressed (amended into commit 91231dc)
Both fixes verified on the test server ( |
1 participant
Summary
Closes HULY-8. The original bug report was that
huly issue updaterejected--labelwhilehuly issue createaccepted it. After fixing that, I audited everycreate/updatecommand pair in the CLI (21 surfaces, ~80 flag pairs) and found the same create/update asymmetry class on 6 additional flags across 4 surfaces:updatehuly issue update--label <l...>ops.labels = opts.label(mirrors create)huly issue update--kind <ref>ops.kind = await resolveKindByRef(...)(power-user TaskType ref branch from create)huly issue update--body <md>,--body-file <path>readBody()→updateMarkup(), body wins over--description(mirrors createdescriptionSource = body ?? opts.description)huly issue update--due <iso>ops.dueDate = opts.due === '' ? null : parseDate(...)(empty string clears)huly issue-template update--body-file <path>fs.readFile+ops.description, mirrorscreateIssueTemplatelines 165-167huly action update--attached-to <ref>,--attached-to-class <class>removeCollection+addCollection+ WorkSlot migration, which belongs in a dedicatedhuly action movesubcommand (out of scope)huly calendar update--body <md>ops.description = opts.bodywins over--description, mirrorscreateEventline 629Also tidies the "nothing to update" error hints and
--helptext to enumerate all accepted flags, and removes a stale--kindreference in theissue updatehint that advertised a flag the CLI never registered (silent typecheck hole).Out-of-scope follow-up
A separate bug surfaced while filing the scope-expansion comment on HULY-8:
huly comment add/update(and likelydm message send, etc.) pass the raw HTML body string asmessagewithout going throughuploadMarkup, so rich content renders as literal<h1>/<p>/<table>characters. Filed as HULY-20 for separate tracking.Verification
pnpm typecheck✅pnpm test43/43 ✅pnpm build✅http://localhost:7180,test@aaravlabs.com/test) with thehuly_v7_testcompose stack from~/apps/huly-selfhost-fork:huly issue update TSK-1 --kind tracker:taskTypes:Issue→kindset on issuehuly issue update TSK-1 --body-file ./rich.mdand--body "<h1>...</h1>..."→ both renderhuly issue update TSK-1 --due 2026-09-15T00:00:00Z→dueDate: 1789430400000mshuly issue-template update <id> --body-file ./tpl.md→ description updated from filehuly action update <id> --attached-to TSK-1 --attached-to-class tracker:class:Issue→ graceful error with workaroundhuly calendar update <id> --body "<h1>New body</h1><p>From --body flag.</p>"→ description set--title X --label a --label b --due ... --body-file ...→ all apply in one call