Fix/hq audit bugs

[Koopa0]

No description provided.

[codereviewbot-ai]

The use of wg.Go(func() { feedScheduler.Run(ctx) }) is problematic because the standard library's sync.WaitGroup does not provide a Go method. Unless a custom WaitGroup implementation is used, this will result in a compilation error or the scheduler goroutine not being properly tracked. Consider using wg.Add(1) before starting the goroutine and defer wg.Done() inside the goroutine:

wg.Add(1)
go func() {
    defer wg.Done()
    feedScheduler.Run(ctx)
}()

This ensures proper synchronization and shutdown handling.

[codereviewbot-ai]

The unconditional defer feedCollector.Stop() immediately after initialization could lead to a panic if an error occurs later in the setup and the function returns early, as the deferred call will attempt to stop a possibly uninitialized or nil collector. Consider deferring feedCollector.Stop() only after all initialization steps that could fail have completed, or add a nil check in the deferred function.

[codereviewbot-ai]

Potential XSS risk in content rendering

Directive and report content are rendered using Angular interpolation ({{ d.content }} and {{ report.content }}) in the modal detail view. If these fields can contain user-supplied HTML or script, there is a risk of cross-site scripting (XSS) if they are ever rendered with [innerHTML] or similar mechanisms. Ensure that all content is strictly treated as plain text and never rendered as HTML, or sanitize content appropriately before rendering.

Recommendation:

• Confirm that all interpolated content is plain text and never rendered with [innerHTML].
• If HTML rendering is required, use Angular's DomSanitizer and sanitize content before display.

[codereviewbot-ai]

Performance inefficiency: repeated calls to getRelatedReports(d.id)

In the modal detail view, getRelatedReports(d.id) is called in both the @if condition and the @for loop. If getRelatedReports is a non-trivial computation or involves data fetching, this can lead to unnecessary repeated work and degrade performance.

Recommendation:

• Cache the result of getRelatedReports(d.id) in a local variable within the template or component, and reuse it for both the condition and the loop.
• Example:

  @let relatedReports = getRelatedReports(d.id);
  @if (relatedReports.length > 0) {
    @for (report of relatedReports; ...)
  }
  

[codereviewbot-ai]

The getRelatedReports method filters the entire reports array each time it is called. If the reports array is large or this method is invoked frequently (such as within a template loop), this could negatively impact performance.

Recommendation:
If performance profiling indicates this is a bottleneck, consider caching the results or restructuring the data to allow for more efficient lookups, such as indexing reports by in_response_to.

[codereviewbot-ai]

Silent Error Handling for Side Effects

In the advanceComplete function, errors from txDayplan.CompleteByTask and txTasks.ResetRecurring are ignored (lines 164 and 173). This can result in silent failures where side effects are not applied, but the transaction still commits, leading to inconsistent state between tasks and daily plan items or recurring task schedules.

Recommendation:
Handle errors explicitly and decide whether to abort the transaction or at least log the errors and surface them to the caller. For example:

if updated, completeErr := txDayplan.CompleteByTask(ctx, taskID, today); completeErr != nil {
    // handle or log error
}

Failing silently should be avoided for critical side effects.

[codereviewbot-ai]

Ambiguous Position Assignment in planDay

The logic for assigning pos (lines 255-257) defaults to the loop index if item.Position is zero. However, zero may be a valid position (i.e., the first item in the plan). This can lead to incorrect ordering if the caller intends to set the position to zero.

Recommendation:
Use a pointer or a sentinel value (e.g., -1) to distinguish between an unset position and a valid zero value. For example:

if item.Position < 0 {
    pos = i
} else {
    pos = item.Position
}

This ensures that zero is treated as a valid position.

[codereviewbot-ai]

⚠️ Potential Data Race: Concurrent Writes to Struct Fields

In fillMorningSections, each section is launched in a separate goroutine and writes to a field of the shared out struct. While the comment claims that fields are disjoint and thus no mutex is needed, Go's memory model does not guarantee safe concurrent writes to different fields of the same struct without explicit synchronization. This can lead to data races and undefined behavior, especially if the struct is accessed elsewhere concurrently (e.g., for logging or inspection).

Recommended Solution:

• Use a mutex to protect writes to the out struct, or
• Refactor to collect results in local variables within each goroutine and assign them to the struct after all goroutines complete, or
• Use a sync/atomic.Value for each field if performance is critical and fields are large.

Example (using local variables):

var (
    overdueTasks []task.PendingTaskDetail
    // ... other fields
)
runSection("tasks", func(c context.Context) { overdueTasks = s.fillMorningTasks(c, date) })
// ...
out.OverdueTasks = overdueTasks

Alternatively, run the section fills sequentially if the performance benefit of concurrency is not significant.

[codereviewbot-ai]

⚠️ Possible Duplicate Directives in Output

In fillDirectives, the code appends results from both UnackedForTarget and UnresolvedForTarget (and similarly for issued directives) to the same output slices. If a directive appears in both queries, it will be duplicated in the output, which may confuse consumers and lead to incorrect downstream processing.

Recommended Solution:

• Deduplicate directives before appending to the output slices. This can be done by tracking directive IDs in a map and only appending unique entries.

Example:

seen := map[string]struct{}{}
for _, d := range dirs {
    if _, ok := seen[d.ID]; !ok {
        out.DirectivesReceived = append(out.DirectivesReceived, d)
        seen[d.ID] = struct{}{}
    }
}

Apply similar logic for issued directives.

[codereviewbot-ai]

The manage_content tool is registered with the additive annotation, but its supported actions (such as publish) can be destructive. Using an annotation that does not reflect the most destructive possible action may mislead clients or downstream systems about the tool's side effects, potentially resulting in improper usage or lack of necessary safeguards.

Recommendation:

• Consider using the destructive annotation for this tool, or splitting it into separate tools for destructive and non-destructive actions to ensure accurate annotation and safer usage.

[augmentcode]

🤖 Augment PR Summary

Summary: This PR fixes several HQ audit issues across the backend MCP tools, feed system, and the admin directives UI.

Changes:

• Added a background feed scheduler that periodically fetches enabled feeds by schedule and records each run in flow_runs.
• Added DB indexes to speed up high-priority RSS highlight queries.
• Improved internal content search to exclude archived content (instead of only returning published).
• Updated daily plan auto-updates to return rows affected, allowing MCP to report whether today’s plan item was actually updated.
• Extended directive querying to include “issued by caller” directives and exposed this in morning_context.
• Expanded MCP manage_content with list, read, and bookmark_rss, and switched outputs to a richer content payload.
• Added validation in plan_day to ensure tasks exist and aren’t still in inbox status.
• Enhanced goal_progress to support status filtering, and normalized priority parsing to accept more inputs.
• Upgraded the admin directives page to support selecting a directive and viewing details in a modal (including related reports).
• Tuned golangci-lint settings to accommodate large MCP input structs passed by value.

Technical Notes: The feed scheduler runs on a 15-minute tick and uses best-effort audit recording; MCP tool docs were updated to reflect the expanded tool surface area.

_{🤖 Was this summary useful? React with 👍 or 👎}

[augmentcode]

Review completed. 1 suggestion posted.

Comment augment review to trigger a new review at any time.

[augmentcode]

This doc says empty search_knowledge results mean “not found in published content”, but the backend internal search was changed to status != 'archived' (so drafts/review can also match). That makes the guidance misleading for users diagnosing empty results.

Other locations where this applies: skills/koopa-system/references/research.md:126

Severity: low

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

[augmentcode]

Review completed. 1 suggestion posted.

Comment augment review to trigger a new review at any time.

[augmentcode]

getRelatedReports(d.id) is called twice from the template, which re-filters on every change detection and can potentially diverge between the @if and @for if reports() changes. Consider caching this as a computed value instead of a method call. (Rule: frontend/AGENTS.md)

Severity: low

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

[augmentcode]

Review completed. 1 suggestion posted.

Comment augment review to trigger a new review at any time.

[augmentcode]

slug generation for bookmark_rss is a simple space→hyphen replacement, so RSS entries with identical/similar titles can easily collide and cause CreateContent to fail with a duplicate-slug conflict. It may also leave punctuation in the slug, producing awkward URLs compared to a dedicated slugify routine.

Severity: medium

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

[augmentcode]

Review completed. 1 suggestion posted.

Comment augment review to trigger a new review at any time.

[augmentcode]

Inside this button block, the lifecycle badge uses Tailwind class rounded-sm, but the repo’s Tailwind v4 rules expect rounded-xs (and reserve rounded-sm for the old rounded). This can lead to inconsistent border radius across the UI. (Rule: frontend/AGENTS.md)

Other locations where this applies: frontend/src/app/admin/commitments/directives.html:217, frontend/src/app/admin/commitments/directives.html:331

Severity: low

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}