feat(missions): per-mission taskPrefix override for triaged task ids#2347
Open
gsxdsm wants to merge 17 commits into
Open
feat(missions): per-mission taskPrefix override for triaged task ids#2347gsxdsm wants to merge 17 commits into
gsxdsm wants to merge 17 commits into
Conversation
Add an optional `taskPrefix` field on Mission so a single mission's tickets can use a distinct id prefix (e.g. ERR-) while the rest of the board keeps the project-wide prefix (default FN-). Previously taskPrefix was project-scoped only; flipping it changed every new ticket board-wide including autopilot. The distributed id allocator was already multi-prefix, so this only threads a per-mission prefix through triage: - Mission.taskPrefix (nullable) persisted via a new additive migration (v140). - MissionStore.triageFeature passes it into TaskCreateInput.taskPrefix (a transient minting hint, not persisted on the task). - createTaskWithDistributedReservation prefers input.taskPrefix over settings. - buildCommitMsgTrailerHook derives the strip PREFIX from the task id itself rather than the project-wide option, so ERR-5 strips correctly while the project prefix is still FN (fixes all engine call sites at once). - Mission create + edit UI (MissionManager) exposes a Task prefix input; mission-routes POST/PATCH validate it (letter-led alphanumeric, uppercased). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-prefix # Conflicts: # packages/core/src/db.ts
…ck prefix Address greptile feedback on PR #1930: - Mission edit save sends taskPrefix:null when the field is cleared so JSON retains the key and PATCH clears the stored mission override. - Commit-msg hook quotes "$PREFIX" in case and escapes sed ERE metacharacters on the options.taskPrefix fallback path.
…efix Rebase the feat/per-mission-task-prefix work onto main after the SQLite→PostgreSQL cutover. Take main's stubbed db.ts and modular store.ts, then re-port the feature: - missions.task_prefix column (schema + migration 0008 + schema-applier) - AsyncMissionStore CRUD/upsert/triage threads Mission.taskPrefix - createTaskBackend + createTaskWithDistributedReservation honor input.taskPrefix - Keep greptile fixes: null PATCH clear, commit-msg prefix escape/quote - Port mission taskPrefix tests to PG harness; mock dashboard route tests Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
escapeSedEre left `/` raw, so a fallback prefix like TEAM/API produced invalid sed `s/^TEAM/API-//i` and could abort the commit-msg hook. Escape `/` with the other ERE metacharacters; cover with a unit test. Greptile: databaseId 3583850800 on PR #1930.
MissionManager state used mission-types.MissionWithSummary while fetchMissions returned api/legacy.MissionWithSummary. taskPrefix was string vs string|null, so the Array.isArray ternary widened to a dual-array union and failed setState (TS2345). Align component Mission.taskPrefix with the API and annotate the normalized list as MissionWithSummary[].
JSON.stringify produced PREFIX="$(id)" which /bin/sh expands via command substitution when the commit-msg hook runs. Emit POSIX single-quoted literals via shellSingleQuote (embedded ' → '\''). Apply the same quoting to TRAILER_NAME and CO_AUTHOR_TRAILER. Cover $(id), ; rm -rf, and O'Brien. Greptile P1 security: databaseIds 3583890011, 3583943640 on PR #1930.
…to 0028 Main claimed migration 0026 (bigint counters) and 0027 (workflow IR pin). Keep the mission task_prefix feature as 0028, wire it after those migrations, update the 0000 baseline, and restore missions.ts taskPrefix on the code-org API surface after legacy.ts re-exports.
Contributor
|
Important Review skippedToo many files! This PR contains 457 files, which is 307 over the limit of 150. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (460)
You can disable this status message by setting the 📝 WalkthroughWalkthroughChangesMission task prefixes can now be configured per mission, persisted in PostgreSQL, validated and edited in the dashboard, propagated during triage, used for task ID allocation, and safely incorporated into generated commit hooks. Mission task prefix support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MissionManager
participant MissionRoutes
participant MissionStore
participant TaskStore
participant CommitHook
MissionManager->>MissionRoutes: submit taskPrefix
MissionRoutes->>MissionStore: create or update mission
MissionStore-->>MissionManager: mission with taskPrefix
MissionStore->>TaskStore: triage feature with taskPrefix
TaskStore-->>MissionStore: prefixed task ID
CommitHook->>CommitHook: derive and escape task prefix
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
Collaborator
Author
|
Conflict-resolved re-land of #2334. Primary checkout remains on |
Contributor
Greptile SummaryThis PR adds an optional per-mission
Confidence Score: 5/5The change is safe to merge: it is strictly additive (nullable column, no backfill), the migration is independently bookmarked so upgrade and fresh-install paths diverge correctly, and all three data paths (create, update-set, update-clear) are covered by integration and route-level tests. Both issues flagged in previous review rounds are addressed. The null-clear distinction (PATCH null to DB NULL) is correctly implemented and verified by route and component tests. The commit-hook shell-injection vectors are closed via single-quoting and ERE escaping. No new correctness or security issues found. No files require special attention. All changed files have clear, consistent handling of the nullable prefix field. Important Files Changed
Reviews (3): Last reviewed commit: "refactor(missions): share resolveTaskPre..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Delete fusion_schema_migrations version 0028 (not 0026) when simulating a pre-mission-prefix cluster so applySchemaBaseline re-applies the migration. Map row.taskPrefix with ?? like other nullable text fields.
Collaborator
Author
Review feedback addressed in
|
Match async-mission-store-queries rowToMission so empty-string rows stay distinguishable if validation ever relaxes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/task-store/remaining-ops-4.ts (1)
178-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared per-mission task-prefix resolution logic. Both call sites independently reimplement
input.taskPrefix?.trim() || settings.taskPrefix || FALLBACK).trim().toUpperCase(); a single helper (parameterized by fallback) would remove the duplication and prevent the two sites from drifting when this rule changes again.
packages/core/src/task-store/remaining-ops-4.ts#L178-L179: replace the inline expression with a call to a sharedresolveTaskPrefix(input.taskPrefix, settings.taskPrefix, "FN")helper.packages/core/src/task-store/task-creation.ts#L196-L197: replace the inline expression with a call to the same shared helper, passing"KB"as the fallback.♻️ Proposed shared helper
+// e.g. in a shared task-id-allocation module +export function resolveTaskPrefix( + taskPrefixHint: string | undefined, + settingsTaskPrefix: string | undefined, + fallback: string, +): string { + return (taskPrefixHint?.trim() || settingsTaskPrefix || fallback).trim().toUpperCase(); +}- const prefix = (input.taskPrefix?.trim() || settings.taskPrefix || "FN").trim().toUpperCase(); + const prefix = resolveTaskPrefix(input.taskPrefix, settings.taskPrefix, "FN");- const prefix = (input.taskPrefix?.trim() || settings.taskPrefix || "KB").trim().toUpperCase(); + const prefix = resolveTaskPrefix(input.taskPrefix, settings.taskPrefix, "KB");🤖 Prompt for 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. In `@packages/core/src/task-store/remaining-ops-4.ts` around lines 178 - 179, Extract the duplicated task-prefix normalization into a shared resolveTaskPrefix helper, parameterized by the input prefix, settings prefix, and fallback. In packages/core/src/task-store/remaining-ops-4.ts:178-179, replace the inline expression with resolveTaskPrefix(input.taskPrefix, settings.taskPrefix, "FN"); in packages/core/src/task-store/task-creation.ts:196-197, make the same replacement using "KB". Preserve trimming, fallback selection, and uppercase normalization.
🤖 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.
Nitpick comments:
In `@packages/core/src/task-store/remaining-ops-4.ts`:
- Around line 178-179: Extract the duplicated task-prefix normalization into a
shared resolveTaskPrefix helper, parameterized by the input prefix, settings
prefix, and fallback. In
packages/core/src/task-store/remaining-ops-4.ts:178-179, replace the inline
expression with resolveTaskPrefix(input.taskPrefix, settings.taskPrefix, "FN");
in packages/core/src/task-store/task-creation.ts:196-197, make the same
replacement using "KB". Preserve trimming, fallback selection, and uppercase
normalization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 20d6742c-16a1-4ba9-87ab-d6f89e1dd4e1
📒 Files selected for processing (23)
.changeset/mission-task-prefix.mdpackages/core/src/__tests__/postgres/mission-store.pg.test.tspackages/core/src/__tests__/postgres/schema-applier.test.tspackages/core/src/async-mission-store-queries.tspackages/core/src/async-mission-store.tspackages/core/src/mission-store.tspackages/core/src/mission-types.tspackages/core/src/postgres/index.tspackages/core/src/postgres/migrations/0000_initial.sqlpackages/core/src/postgres/migrations/0028_mission_task_prefix.sqlpackages/core/src/postgres/schema-applier.tspackages/core/src/postgres/schema/project.tspackages/core/src/task-store/remaining-ops-4.tspackages/core/src/task-store/task-creation.tspackages/core/src/types.tspackages/dashboard/app/api/missions.tspackages/dashboard/app/components/MissionManager.tsxpackages/dashboard/app/components/__tests__/MissionManager.task-prefix.test.tsxpackages/dashboard/app/components/mission-types.tspackages/dashboard/src/__tests__/mission-task-prefix-routes.test.tspackages/dashboard/src/mission-routes.tspackages/engine/src/__tests__/worktree-hooks.test.tspackages/engine/src/worktree-hooks.ts
Extract the input → settings → fallback prefix normalization used by createTaskWithDistributedReservation (FN) and createTaskBackend (KB) so the two sites cannot drift (CodeRabbit #2347).
Collaborator
Author
CodeRabbit review body addressed in
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/core/src/task-store/__tests__/task-prefix.test.ts`:
- Around line 14-17: Add coverage in the resolveTaskPrefix tests for
whitespace-only settings values, asserting they fall back to the configured
default prefix rather than returning an empty result. Keep the existing
blank-hint cases and verify the general fallback behavior with a settings
argument containing only whitespace.
In `@packages/core/src/task-store/task-prefix.ts`:
- Line 13: Update the task-prefix resolution expression to trim
settingsTaskPrefix before applying precedence, so whitespace-only settings are
treated as unset and fallback is selected; preserve the existing uppercase
normalization and ensure both allocation callers receive a non-empty resolved
prefix. Add a regression test covering whitespace-only settingsTaskPrefix.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ccd6f486-dd65-44db-b589-65644e4e2978
📒 Files selected for processing (4)
packages/core/src/task-store/__tests__/task-prefix.test.tspackages/core/src/task-store/remaining-ops-4.tspackages/core/src/task-store/task-creation.tspackages/core/src/task-store/task-prefix.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/task-store/remaining-ops-4.ts
- packages/core/src/task-store/task-creation.ts
Comment on lines
+14
to
+17
| it("falls back to settings when the input hint is blank", () => { | ||
| expect(resolveTaskPrefix(" ", "fn-board", "KB")).toBe("FN-BOARD"); | ||
| expect(resolveTaskPrefix(undefined, "fn-board", "KB")).toBe("FN-BOARD"); | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add coverage for blank settings values.
The suite tests blank hints but not whitespace-only settings, so it would not catch the helper returning an empty prefix instead of the fallback.
Proposed test
it("falls back to settings when the input hint is blank", () => {
expect(resolveTaskPrefix(" ", "fn-board", "KB")).toBe("FN-BOARD");
expect(resolveTaskPrefix(undefined, "fn-board", "KB")).toBe("FN-BOARD");
+ expect(resolveTaskPrefix(undefined, " ", "KB")).toBe("KB");
});As per coding guidelines, regression tests should assert the general invariant and verify the original symptom is gone.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("falls back to settings when the input hint is blank", () => { | |
| expect(resolveTaskPrefix(" ", "fn-board", "KB")).toBe("FN-BOARD"); | |
| expect(resolveTaskPrefix(undefined, "fn-board", "KB")).toBe("FN-BOARD"); | |
| }); | |
| it("falls back to settings when the input hint is blank", () => { | |
| expect(resolveTaskPrefix(" ", "fn-board", "KB")).toBe("FN-BOARD"); | |
| expect(resolveTaskPrefix(undefined, "fn-board", "KB")).toBe("FN-BOARD"); | |
| expect(resolveTaskPrefix(undefined, " ", "KB")).toBe("KB"); | |
| }); |
🤖 Prompt for 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.
In `@packages/core/src/task-store/__tests__/task-prefix.test.ts` around lines 14 -
17, Add coverage in the resolveTaskPrefix tests for whitespace-only settings
values, asserting they fall back to the configured default prefix rather than
returning an empty result. Keep the existing blank-hint cases and verify the
general fallback behavior with a settings argument containing only whitespace.
Source: Coding guidelines
| settingsTaskPrefix: string | undefined, | ||
| fallback: string, | ||
| ): string { | ||
| return (taskPrefixHint?.trim() || settingsTaskPrefix || fallback).trim().toUpperCase(); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Treat whitespace-only settings as unset.
settingsTaskPrefix is checked before trimming. For " ", this returns "" instead of the fallback, and both downstream allocation paths can receive an empty prefix. Trim each candidate before applying precedence and add a regression test.
The supplied callers pass this value directly to distributed task-ID allocation.
🤖 Prompt for 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.
In `@packages/core/src/task-store/task-prefix.ts` at line 13, Update the
task-prefix resolution expression to trim settingsTaskPrefix before applying
precedence, so whitespace-only settings are treated as unset and fallback is
selected; preserve the existing uppercase normalization and ensure both
allocation callers receive a non-empty resolved prefix. Add a regression test
covering whitespace-only settingsTaskPrefix.
Contributor
|
I resolved the current Direct push to this PR branch was rejected with a 403, so #2365 targets |
Resolves the current `main` conflicts in #2347. Key resolution: - preserves `main`'s task declared-symbol migration as `0028` - renumbers the mission task-prefix migration to `0029` to avoid duplicate migration bookkeeping identities - preserves both task-creation imports and both migration-version expectations Validation: - core, dashboard, and engine typechecks passed - mission prefix core/dashboard/engine focused tests passed - live PostgreSQL run: 99/100 tests passed; the sole failure is the pre-existing `0026_bigint_counters.sql` fixture failure in the `0000` automation-upgrade test --------- Co-authored-by: gsxdsm <gsxdsm@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Fusion (runfusion.ai) <noreply@runfusion.ai> Co-authored-by: Phil Larson <hello@phillarson.xyz> Co-authored-by: v <v@v.speedport.ip>
Contributor
|
Too many files changed for review. ( Bypass the limit by tagging |
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
2 participants
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.
Summary
Maintainer re-land of #2334 (fork
flexi767:feat/per-mission-task-prefix) after resolving merge conflicts with currentmain.Fork push was unavailable despite
maintainerCanModify, so this branch carries the conflict resolution.Feature
taskPrefixfor triaged task ids (inherits project prefix when unset)project.missions.task_prefixConflict resolution
0000_initial.sqlincludestask_prefixon missionslegacy.tskeeps code-org re-exports;missions.tscarriestaskPrefixon create/update typesTest plan
Closes / supersedes #2334 once this lands (or re-point the fork PR).
Summary by CodeRabbit