Skip to content

fix: sync-progress reports already-synced for an in-progress anchor - #296

Merged
bborbe merged 2 commits into
masterfrom
fix/sync-progress-inprogress
Oct 15, 2026
Merged

bborbe merged 2 commits into
masterfrom
fix/sync-progress-inprogress

Conversation

@bborbe

@bborbe bborbe commented Oct 15, 2026

Copy link
Copy Markdown
Owner

What

/vault-cli:sync-progress § Phase 2 now reaches its already synced — no writes this run short-circuit for an in_progress anchor, instead of reporting failure for a record it can itself verify as present.

Why

The short-circuit added in v0.168.6 was gated "When a completion was established above". An anchor that is still open — the state the command is most often run in — fell past it to:

- Neither PR nor completion → `No task completion or PR detected. Use /update instead for in-progress work.` and STOP.

Measured 2026-10-15: the operator ran the command four times in one session against an in_progress anchor whose daily-note entry already existed. All four stopped at that same line; each cost a full Phase 1–2 sweep (vault config read, session-transcript scan, git probes) and produced an identical report. The line is also wrong on that input — the record exists, and the command's own representation check can prove it.

The change

Three lines in commands/sync-progress.md:

+- Anchor is **`in_progress`** → **do not stop here.** Apply the short-circuit below first: run the representation check, and when it passes report `already synced — no writes this run` and STOP. When it fails, fall through to the line below. …
 - Neither PR nor completion → `No task completion or PR detected. Use /update instead for in-progress work.` and STOP.
 
-**Short-circuit when the record already exists.** When a completion was established above, apply …
+**Short-circuit when the record already exists.** Apply this whenever a completion was established above **or** the anchor is `in_progress`, using …

Acceptance reading (taken on the branch, before this PR)

$ grep -n 'Apply this whenever a completion was established above' commands/sync-progress.md
134:**Short-circuit when the record already exists.** Apply this whenever a completion was established above **or** the anchor is `in_progress`, …

$ grep -n 'Neither PR nor completion' commands/sync-progress.md
132:- Neither PR nor completion → `No task completion or PR detected. Use /update instead for in-progress work.` and STOP.

Still keyed on the representation check, not on the status alone: an in_progress anchor with no daily-note entry falls through to the STOP line unchanged.

Local review

  • make precommit — PASS (all stages green: generate, unit + integration tests, scenario rules, golangci-lint 0 issues, vet, osv-scanner, trivy, CHANGELOG structure, licenses).
  • /coding:pr-review (selector mode) — 0 mechanical findings across 79 rule-yamls; 1 candidate judgment rule adjudicated clean.

Out of scope

  • The Phase 1–2 sweep itself. The short-circuit still runs after the completion analysis, so the sweep is not removed — what is removed is the false failure report and the operator's reason to re-run.
  • The close-family loop in /vault-cli:session-close.
  • The update-task route the STOP line points at — its wording and behaviour are unchanged.

@bborbe
bborbe marked this pull request as ready for review October 15, 2026 16:22

@ben-s-pull-request-reviewer ben-s-pull-request-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The CHANGELOG entry is properly under ## Unreleased with a conventional-prefix fix: and comprehensive description.

Review summary:

Both changed files are correct:

  • CHANGELOG.md — ## Unreleased section added with a detailed fix description, conventional fix: prefix
  • commands/sync-progress.md — The in_progress arm (line 131) correctly precedes the STOP bullet (line 132). The short-circuit gate (line 134) now reads "whenever a completion was established above or the anchor is in_progress". The fallthrough path is preserved: representation check fails → falls through to the STOP line.
{
  "verdict": "approve",
  "summary": "Bugfix correctly adds an `in_progress` arm before the STOP bullet so an `in_progress` anchor hits the representation check first; the short-circuit gate is extended to fire for `in_progress` anchors too. Control flow ordering concern is resolved.",
  "comments": [],
  "concerns_addressed": [
    {
      "concern": "correctness: control flow ordering — new `in_progress` arm must be evaluated before the STOP bullet",
      "disposition": "not-an-issue",
      "detail": "Verified in worktree: `in_progress` arm (line 131) precedes the STOP bullet (line 132), short-circuit gate (line 134) extended to include `in_progress`, and fallthrough path when representation check fails is preserved"
    }
  ]
}

@ben-s-pull-request-reviewer
ben-s-pull-request-reviewer Bot dismissed their stale review October 15, 2026 16:37

superseded by new automated review

@ben-s-pull-request-reviewer ben-s-pull-request-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Now I have everything for Step 4c-sel CLASSIFY and 4d-sel ADJUDICATE.


Step 4c-sel: CLASSIFY

Candidates: 13 judgment rules from Step 4b-i glob filter

Rule Applies When Result
agent-cmd/command-frontmatter any commands/*.md file is created SKIPPED — commands/sync-progress.md already existed; not created
agent-cmd/command-thin any new commands/*.md file is added or substantially changed SKIPPED — file is not new; doc-only changes (clarifying arm, gate wording) are not "substantially changed" per the rule's intent
agent-cmd/gap-driven-feedback an agent depends on documented information that may be incomplete SKIPPED — no agent dependency on incomplete docs in this diff
agent-cmd/no-user-prompts agent/command performs work that could prompt the user SKIPPED — no user-prompting behavior
agent-cmd/scripts-in-claude-dir an agent depends on executable scripts SKIPPED — no script dependency
agent-cmd/single-source-of-truth agent's domain has multiple data sources SKIPPED — not relevant to this diff
changelog/conventional-prefix-required ## Unreleased bullet lacks conventional prefix APPLICABLE — CHANGELOG.md is changed
git-commit/feature-branch-no-tag git tag vX.Y.Z on a feature branch SKIPPED — no tag command in diff
git-commit/imperative-mood git commit subject non-imperative SKIPPED — rule governs commit messages, not CHANGELOG bullets
markdown-todo/lowercase-x-for-complete markdown checkbox uses [X] not [x] SKIPPED — no checkbox in diff
go-security/resource-ownership Go source touched SKIPPED — no Go source files in diff
go-security/tenant-isolation Go source touched SKIPPED — no Go source files in diff
git-workflow/never-direct-commit-to-master direct commit to master SKIPPED — no such commit in diff
{
  "applicable": ["changelog/conventional-prefix-required"],
  "skipped": {
    "agent-cmd/command-frontmatter": "file already existed, not created",
    "agent-cmd/command-thin": "file not new, doc-only changes",
    "agent-cmd/gap-driven-feedback": "no agent dependency on incomplete docs",
    "agent-cmd/no-user-prompts": "no user-prompting behavior in diff",
    "agent-cmd/scripts-in-claude-dir": "no script dependency in diff",
    "agent-cmd/single-source-of-truth": "not applicable to this diff",
    "git-commit/feature-branch-no-tag": "no tag command in diff",
    "git-commit/imperative-mood": "rule governs commit msgs, not CHANGELOG",
    "markdown-todo/lowercase-x-for-complete": "no markdown checkbox in diff",
    "go-security/resource-ownership": "no Go source files in diff",
    "go-security/tenant-isolation": "no Go source files in diff",
    "git-workflow/never-direct-commit-to-master": "no direct commit to master"
  }
}

Step 4d-sel: ADJUDICATE

Applicable rule: changelog/conventional-prefix-required

The rule requires ## Unreleased bullets to start with feat:, fix:, refactor:, test:, docs:, chore:, or perf:. The diff adds:

- fix: `/vault-cli:sync-progress` § Phase 2 reaches its `already synced — no writes this run` short-circuit for an `in_progress` anchor, instead of reporting failure...

The bullet starts with fix: — compliant.

No other applicable rules produced violations.

Citation validation: the sole finding references changelog/conventional-prefix-required which exists in rules/index.json — valid.


Step 5: Consolidated Report

Must Fix (Critical): None.
Should Fix (Important): None.
Nice to Have (Optional): None.


Traceability Report (selector mode)

  • Candidates: 13 rules matched by Step 4b-i glob filter
  • Applicable: 1 rule selected by Step 4c-sel (changelog/conventional-prefix-required)
  • Skipped: 12 rules — see classification table above
  • Mechanical findings: 0 findings from Step 4a (clean funnel)
  • Selector result: selector clean — no adjudication needed (1 applicable rule, 0 violations found)

Step 7: Manual Review of Changed Code

CHANGELOG.md: One new bullet under ## Unreleased, correctly prefixed fix:. The changelog write-up is detailed and accurate — documents the bug, the user-visible symptom (4 re-runs), the fix mechanism, and the out-of-scope boundaries.

commands/sync-progress.md: Two changes:

  1. A new explicit in_progress arm inserted between the completion-analysis arm and the terminal STOP gate. This arm redirects to the short-circuit rather than stopping — correctly placed.
  2. The short-circuit gate condition is extended from "when a completion was established above" to "whenever a completion was established above or the anchor is in_progress". The script resolution note (plugin root, not cwd) is retained verbatim.

Both concerns from the plan were verified:

  • ✅ The in_progress arm correctly precedes the STOP line; an in_progress anchor with no daily-note entry falls through to STOP unchanged.
  • ✅ The short-circuit condition now fires for in_progress anchors, avoiding the full Phase 1–2 sweep on re-runs.
  • ✅ The representation check resolution (plugin root) is unchanged and correct.

Now emitting the verdict JSON:

{
  "verdict": "approve",
  "summary": "Clean bugfix: two-file change — a CHANGELOG bullet (correctly prefixed `fix:`) and a command-doc clarification that extends the Phase 2 short-circuit to fire for `in_progress` anchors, eliminating a false-failure report the command could self-resolve. No rule violations, no blockers.",
  "comments": [],
  "concerns_addressed": [
    {
      "concern": "correctness: in_progress arm fallthrough — in_progress with no daily-note entry must still reach the STOP line",
      "disposition": "addressed",
      "detail": "The new explicit in_progress arm at commands/sync-progress.md:131 runs the representation check first; only on check-pass does it STOP. A check-fail (no daily-note entry) falls through to the next line — the original STOP bullet — unchanged."
    },
    {
      "concern": "correctness: representation check resolved under plugin root, not cwd",
      "disposition": "addressed",
      "detail": "The short-circuit gate retains the same script-resolution note: uses session-close § Phase 2 route 1 exception, resolved under plugin root via VAULT_CLI_SCRIPTS. This is consistent with the existing Phase 7 pattern."
    },
    {
      "concern": "performance: short-circuit now fires for in_progress anchors, avoiding Phase 1-2 sweep on re-runs",
      "disposition": "addressed",
      "detail": "The gate condition is extended from 'when completion was established above' to 'whenever completion was established above OR anchor is in_progress', so an already-synced in_progress anchor short-circuits before Phase 1-2 runs."
    }
  ]
}

@bborbe
bborbe merged commit a87e548 into master Oct 15, 2026
3 checks passed
@bborbe
bborbe deleted the fix/sync-progress-inprogress branch October 15, 2026 16:38
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.

1 participant