Skip to content

Feature/task store abstraction#77

Open
bbolek-ap wants to merge 9 commits intoandresharpe:mainfrom
bbolek-ap:feature/task_store_abstraction
Open

Feature/task store abstraction#77
bbolek-ap wants to merge 9 commits intoandresharpe:mainfrom
bbolek-ap:feature/task_store_abstraction

Conversation

@bbolek-ap
Copy link
Contributor

Resolves: https://github.com/andresharpe/dotbot/blob/claude/dotbot-refactor-planning-kKO9U/docs/DOTBOT-V4-phase-02-taskstore-abstraction.md

This pull request introduces a new centralized TaskStore.psm1 module for atomic task state transitions and CRUD operations, and refactors the task-marking tools (task-mark-analysing, task-mark-analysed, and task-mark-done) to use this module for all task file operations. The changes simplify and standardize task state management, reduce duplicated code, and improve reliability and maintainability.

Task state management refactoring:

  • Added the TaskStore.psm1 module, which provides atomic task state transitions (Move-TaskState) and CRUD operations (Get-TaskByIdOrSlug, New-TaskRecord, Update-TaskRecord) for tasks, centralizing all task filesystem I/O and enforcing validated transitions.
  • Refactored task-mark-analysing and task-mark-analysed scripts to use Move-TaskState for moving tasks between states, replacing manual file operations and custom property updates with standardized, atomic transitions. [1] [2]
  • Refactored task-mark-done script to import and prepare for using the new TaskStore.psm1 module, improving consistency across task-marking tools.

Code simplification and reliability improvements:

  • Removed duplicated helper functions and manual directory/file handling from task-marking scripts, replacing them with calls to TaskStore.psm1 functions for property updates, directory creation, and file movement. [1] [2]
  • Improved idempotency and error handling in task-marking scripts by leveraging Move-TaskState's built-in checks for already-in-state and missing tasks, and standardized return values for downstream processing. [1] [2]

Minor improvements:

  • Simplified string concatenation in diagnostic activity log writing for task-mark-done, making error messages more readable and consistent.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a centralized PowerShell TaskStore.psm1 module to standardize task CRUD and state transitions, and refactors multiple MCP task-marking tools to use it, with accompanying test coverage added to the repo’s integration test suite.

Changes:

  • Added TaskStore.psm1 with Move-TaskState, Get-TaskByIdOrSlug, New-TaskRecord, and Update-TaskRecord.
  • Refactored task-marking tools to use Move-TaskState instead of custom filesystem/JSON manipulation.
  • Added a new Test-TaskStore.ps1 test suite and wired it into Run-Tests.ps1; added a timestamp-compare helper for tests.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/Test-TaskStore.ps1 Adds source-backed module tests for TaskStore transitions + CRUD.
tests/Test-Helpers.psm1 Adds Compare-IsoTimestamps helper and exports it for tests.
tests/Run-Tests.ps1 Runs the new TaskStore tests as part of Layer 2.
profiles/default/systems/mcp/tools/task-mark-todo/script.ps1 Refactors todo transition to use Move-TaskState.
profiles/default/systems/mcp/tools/task-mark-skipped/script.ps1 Refactors skipped transition to use TaskStore (but currently has an idempotency regression).
profiles/default/systems/mcp/tools/task-mark-needs-input/script.ps1 Refactors needs-input transition to TaskStore; keeps notification + session handling.
profiles/default/systems/mcp/tools/task-mark-in-progress/script.ps1 Refactors in-progress transition to TaskStore; keeps session tracking updates.
profiles/default/systems/mcp/tools/task-mark-done/script.ps1 Refactors done transition to TaskStore; keeps verification, commit info, and activity log capture.
profiles/default/systems/mcp/tools/task-mark-analysing/script.ps1 Refactors analysing transition to TaskStore; keeps session tracking updates.
profiles/default/systems/mcp/tools/task-mark-analysed/script.ps1 Refactors analysed transition to TaskStore; keeps activity log + session closure.
profiles/default/systems/mcp/modules/TaskStore.psm1 New module implementing task lookup, state transitions, and CRUD operations.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +76 to +100
param(
[Parameter(Mandatory)] [string]$TaskId,
[Parameter(Mandatory)] [string[]]$FromStates,
[Parameter(Mandatory)] [string]$ToState,
[hashtable]$Updates = @{}
)

$baseDir = Get-TaskStoreBaseDir

# Idempotent: already in target state
$existing = Find-TaskFile -TaskId $TaskId -InStatuses @($ToState) -BaseDir $baseDir
if ($existing) {
return @{
task = $existing.Content
old_status = $ToState
new_status = $ToState
old_path = $existing.File.FullName
new_path = $existing.File.FullName
already_in_state = $true
}
}

# Find in allowed source states
$found = Find-TaskFile -TaskId $TaskId -InStatuses $FromStates -BaseDir $baseDir
if (-not $found) {
Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move-TaskState never validates -ToState / -FromStates against the module’s ValidStatuses list. A typo (or unexpected value) will silently create a new directory under tasks and/or make the task undiscoverable by Get-TaskByIdOrSlug (which only scans ValidStatuses). Add validation (e.g., throw if any state is not in ValidStatuses) before doing filesystem operations.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bbolek-ap this one makes sense so that we control what "task statuses" we allow.

Comment on lines +244 to +250
$task = $found.task
$blockedFields = @('id', 'created_at')

foreach ($key in $Updates.Keys) {
if ($key -in $blockedFields) { continue }
Set-OrAddProperty -Object $task -Name $key -Value $Updates[$key]
}
Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update-TaskRecord allows updating the status field in-place without moving the file between state directories. That can create an inconsistent task where status disagrees with its directory, and subsequent state transitions/lookups will behave incorrectly. Consider blocking status updates here (forcing state changes through Move-TaskState) or making Update-TaskRecord optionally handle status changes via Move-TaskState.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bbolek-ap I think this one is worth having a look and fixing

-Updates @{
skip_history = $skipHistory
}

Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

task-mark-skipped appends to $skipHistory and then calls Move-TaskState, but if the task is already in skipped state, Move-TaskState returns already_in_state = $true and does not apply the skip_history update. This regresses the previous behavior where re-skipping appended to history. Handle the already-in-state case by updating the JSON in place (e.g., via Update-TaskRecord or writing $skipHistory back to $result.new_path).

Suggested change
# If the task was already in the 'skipped' state, Move-TaskState may not apply
# the -Updates hashtable. In that case, update the task JSON in place so that
# skip_history is still persisted with the new entry.
if ($result -and $result.PSObject.Properties['already_in_state'] -and $result.already_in_state) {
$taskPath = $null
if ($result.PSObject.Properties['new_path'] -and $result.new_path) {
$taskPath = $result.new_path
} elseif ($existing.PSObject.Properties['path'] -and $existing.path) {
$taskPath = $existing.path
}
if ($taskPath) {
$taskJson = Get-Content -LiteralPath $taskPath -Raw | ConvertFrom-Json
$taskJson.skip_history = $skipHistory
$taskJson | ConvertTo-Json -Depth 20 | Set-Content -LiteralPath $taskPath
}
}

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bbolek-ap I don't think this one makes sense - if the task is skipped, then we don't need to worry about the skip history (it must have been persisted before). Happy to resolve without fix.

Comment on lines +111 to +112
# Caller-supplied updates (merged in)
foreach ($key in $Updates.Keys) {
Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move-TaskState applies caller-supplied -Updates after setting the standard fields, which means a caller can accidentally (or intentionally) override invariants like status and updated_at and leave the task file in the wrong state/directory. Consider blocking these keys in -Updates (and possibly id/created_at) or applying standard fields after merging updates so status always ends up as -ToState and updated_at is always refreshed.

Suggested change
# Caller-supplied updates (merged in)
foreach ($key in $Updates.Keys) {
# Caller-supplied updates (merged in)
$reservedKeys = @('status', 'updated_at', 'id', 'created_at')
foreach ($key in $Updates.Keys) {
if ($reservedKeys -contains $key) {
continue
}

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bbolek-ap I think this one is worth fixing also.

@bbolek-ap
Copy link
Contributor Author

@carlospedreira Updated The PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants