fix: improve retro report lookup and follow-up tracking - #20
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The legacy lookup still accepts reports with a later Project: line, undermining the exact-match fix.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Captain, this PR improves retro discovery and workflow follow-up tracking.
Changes:
- Adds exact project matching and legacy-report lookup.
- Adds workflow-level follow-up tracking.
- Documents all four proposal routes.
| File | Description |
|---|---|
README.md |
Adds the process-change proposal route. |
.agents/skills/retro/SKILL.md |
Updates retro lookup and follow-up instructions. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| `P='<project>' awk 'FNR == 1 { retro = substr($0, 1, 9) == "# Retro: " } FNR == 2 && retro && $0 == "Project: " ENVIRON["P"] { print FILENAME }' data/*/report.md` | ||
| A home may hold several projects, and another project's follow-ups are not evidence about this one. | ||
| - The earlier retro reports without a `Project:` second line, which were written before this skill: keep each one whose body links into the project's repository, and name every report found this way in your list of sources. | ||
| `awk 'FNR == 1 { retro = substr($0, 1, 9) == "# Retro: " } FNR == 2 && retro && substr($0, 1, 9) != "Project: " { print FILENAME }' data/*/report.md | while IFS= read -r f; do grep -lF '<repository address>/' "$f"; done`, where `<repository address>` is the repository's web address without its scheme, such as `github.com/<owner>/<repo>`. |
There was a problem hiding this comment.
Fixed in f278a6ae0: the legacy fallback now skips any report that has a Project: line anywhere (grep -q '^Project:') before it checks for a repository link, so a later Project: line no longer lets another project's report through. The check is unchanged at the PR head a62a047.
a98f071 to
88326a6
Compare
zakna
changed the title
zakna
changed the title
…name workflow follow-ups file Refs zakna/fm-tooling#19, zakna/fm-tooling#22, zakna/fm-tooling#23
…sitory identities
c3c64e1 to
a62a047
Compare
zakna
changed the title
Intent
Approved process change from the retros of 2026-10-01: Fix the retro skill lookup of earlier retros, backfill the Project line, fix the README row - fm-tooling 19, 22, 23.
Context: the first three retros run under the /retro skill each found its earlier-retro lookup empty, because no report written before the skill carries a
Project:line; they found earlier retros only by task-id name. Issues: https://github.com/zakna/fm-tooling/issues/19 (the line is matched as a pattern on any line), https://github.com/zakna/fm-tooling/issues/22 (README row names three proposal classes, the skill has four), https://github.com/zakna/fm-tooling/issues/23 (reports without the line are never found; workflow-level follow-ups have no place). The backfill of theProject:line in the 12 older reports of the Mac home is already done outside the repository.What Changed
/retrolocate same-project earlier reports using exact project metadata, while also finding legacy reports without aProject:line when they link to the repository.${FM_HOME}/data/retro-workflow-followups.md, including supervisor ownership and re-verification/removal of completed entries./retroentry to include process-change proposals among its routing targets.Risk Assessment
🚨 High: The core lookup remains unreachable from ordinary scout worktrees, is unsupported under the Rovo access boundary, and the fallback and metrics filters can silently omit or mix historical records.
Testing
Two disposable real-Claude primaries were driven through private lab tmux sessions. The generated retro report found the active-home tagged report and matching legacy report while excluding decoys, and the supervisor handoff persisted both workflow entries in the active home ledger. Three runtime scenarios passed live. The README proposal-route contract was only directly inspected, so its scenario is untested under the live-validation contract. Reviewer-visible artifacts were exported; no repository-wide test suite, linter, or formatter was run.
/retrofor a landed local ticket with a Project-tagged earlier report and an active-home workflow ledger; the generated report lists both sources and re-verifies the workflow entry./retrowith a pre-Project report linking the target repository beside wrong-project and unrelated reports; the generated source list includes the matching legacy report and excludes both decoys./retroadvertise all four proposal routes: automated check, reviewer path rule, navigation line, and process change.README.mdinspection and explicitly marks this scenario live=false, so it does not establish a live result. No live product/runtime authority was supplied for t…Evidence: Live retro lookup report
Source: Live retro lookup report
Evidence: Live workflow ledger
Source: Live workflow ledger
Evidence: Targeted lookup command evidence
Source: Targeted lookup command evidence
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
.agents/skills/retro/SKILL.md:56- The new lookup is rooted at the worker's current directory rather than the active Firstmate home. Scouts run in disposable project worktrees, while durable reports live under the home-selected data directory ($FM_HOME/dataorFM_DATA_OVERRIDE); consequently lines 56 and 59 either find nodatadirectory or scan project-owned data, and the workflow ledger named at lines 33, 60, and 68 is also not reliably read or written in the active home. The supported Rovo path is additionally unable to read sibling reports, the workflow file, or the pipeline database at lines 108-134: its external grant covers only the current task's report/inbox/status and its bash remains worktree-confined. Bind every home path to the active home/override; deciding to widen Rovo's access to private sibling records or adding a trusted home-side reader requires explicit authorization..agents/skills/retro/SKILL.md:59- The legacy fallback only matches<repository address>/, so it silently drops a pre-skill report with no Project line whose only repository evidence is a root link such ashttps://github.com/acme/tool(and equivalent GitLab links). That contradicts the prose requirement to keep every report whose body links into the repository; supported Gerrit change links can likewise use a/c/<project>/+/<number>route rather than the origin-style repository prefix. Match a normalized forge-aware repository URL boundary, accepting both the repository root and descendant issue, review, or change URLs..agents/skills/retro/SKILL.md:113- The metrics query claims to return attempts for this delivery, butWHERE r.branch = ... OR r.pr_url = ...never constrainsr.repo_id; thereposjoin only printsupstream_url. In a multi-project home with a reused ship branch, or for a direct-PR/local-only delivery reusing a branch that previously had no-mistakes runs, unrelated or old runs are returned and the following per-run queries count them as this ticket's M2-M8. Scope the branch match to the delivery's repository identity and use a delivery-specific identity when no PR exists.🔧 Fix applied.
5 issues (2 errors, 3 warnings) still open:
.agents/skills/retro/SKILL.md:56- The new lookup is rooted at the worker's current directory rather than the active Firstmate home. Scouts run in disposable project worktrees, while durable reports live under the home-selected data directory ($FM_HOME/dataorFM_DATA_OVERRIDE); consequently lines 56 and 59 either find nodatadirectory or scan project-owned data, and the workflow ledger named at lines 33, 60, and 68 is also not reliably read or written in the active home. The supported Rovo path is additionally unable to read sibling reports, the workflow file, or the pipeline database at lines 108-134: its external grant covers only the current task's report/inbox/status and its bash remains worktree-confined. Bind every home path to the active home/override; deciding to widen Rovo's access to private sibling records or adding a trusted home-side reader requires explicit authorization..agents/skills/retro/SKILL.md:59- The legacy fallback only matches<repository address>/, so it silently drops a pre-skill report with no Project line whose only repository evidence is a root link such ashttps://github.com/acme/tool(and equivalent GitLab links). That contradicts the prose requirement to keep every report whose body links into the repository; supported Gerrit change links can likewise use a/c/<project>/+/<number>route rather than the origin-style repository prefix. Match a normalized forge-aware repository URL boundary, accepting both the repository root and descendant issue, review, or change URLs..agents/skills/retro/SKILL.md:113- The metrics query claims to return attempts for this delivery, butWHERE r.branch = ... OR r.pr_url = ...never constrainsr.repo_id; thereposjoin only printsupstream_url. In a multi-project home with a reused ship branch, or for a direct-PR/local-only delivery reusing a branch that previously had no-mistakes runs, unrelated or old runs are returned and the following per-run queries count them as this ticket's M2-M8. Scope the branch match to the delivery's repository identity and use a delivery-specific identity when no PR exists..agents/skills/retro/SKILL.md:59- The e0b8d5f fix-round regex still treats the repository address as an unescaped substring rather than a forge-aware URL boundary. For target github.com/acme/tool, an unrelated link such as https://evil.example/github.com/acme/tool/issues/1 is accepted because '/' is allowed before the address, and github.com/acme/tool.v2/... is accepted because '.' is allowed after the repository segment; an address containing a literal dot can also match a different path because the interpolated ERE is not escaped. This imports unrelated retro reports into the project's follow-up set. Escape the literal address and require the repository root or a descendant URL path, applying the same rule to Gerrit; line 59 is the only changed-code site for this invariant. This is a new false-positive defect in the prior round's replacement of the literal fallback..agents/skills/retro/SKILL.md:115- The e0b8d5f repository-scoping fix moved the metrics identity failure from false inclusion to false omission: when a pull-request URL is supplied, the query now accepts only rows with r.pr_url equal to that final URL. A supported no-mistakes delivery can have an attempt fail, cancel, or restart before its PR step, leaving r.pr_url empty, followed by a later attempt that creates or uses the PR and lands. The query then silently drops the earlier attempt despite lines 104-105 requiring every attempt, so M2-M8 undercount time, rounds, findings, gates, and unreconciled work. Preserve a repository-scoped, delivery-specific match for pre-PR branch attempts instead of making the final PR URL their sole identity.🔧 Fix applied.
2 warnings still open:
.agents/skills/retro/SKILL.md:59- The e0b8d5f fix-round regex still treats the repository address as an unescaped substring rather than a forge-aware URL boundary. For target github.com/acme/tool, an unrelated link such as https://evil.example/github.com/acme/tool/issues/1 is accepted because '/' is allowed before the address, and github.com/acme/tool.v2/... is accepted because '.' is allowed after the repository segment; an address containing a literal dot can also match a different path because the interpolated ERE is not escaped. This imports unrelated retro reports into the project's follow-up set. Escape the literal address and require the repository root or a descendant URL path, applying the same rule to Gerrit; line 59 is the only changed-code site for this invariant. This is a new false-positive defect in the prior round's replacement of the literal fallback..agents/skills/retro/SKILL.md:115- The e0b8d5f repository-scoping fix moved the metrics identity failure from false inclusion to false omission: when a pull-request URL is supplied, the query now accepts only rows with r.pr_url equal to that final URL. A supported no-mistakes delivery can have an attempt fail, cancel, or restart before its PR step, leaving r.pr_url empty, followed by a later attempt that creates or uses the PR and lands. The query then silently drops the earlier attempt despite lines 104-105 requiring every attempt, so M2-M8 undercount time, rounds, findings, gates, and unreconciled work. Preserve a repository-scoped, delivery-specific match for pre-PR branch attempts instead of making the final PR URL their sole identity./retroadvertise all four proposal routes: automated check, reviewer path rule, navigation line, and process change./retrofor a landed local ticket with a Project-tagged earlier report and an active-home workflow ledger; the generated report lists both sources and re-verifies the workflow entry./retrowith a pre-Project report linking the target repository beside wrong-project and unrelated reports; the generated source list includes the matching legacy report and excludes both decoys./retroadvertise all four proposal routes: automated check, reviewer path rule, navigation line, and process change.README.mdinspection and explicitly marks this scenario live=false, so it does not establish a live result. No live product/runtime authority was supplied for t…bin/fm-lab-home.sh create "$LAB"followed by a real Claude primary in a privatetmux -L fm-labsession, with same-socket teardown.Live/retroworker execution against an isolatedFM_HOMEcontaining tagged, legacy, wrong-project, unrelated, and workflow-ledger records.The project-tagged and legacy lookup commands from.agents/skills/retro/SKILL.mdagainst an isolated fixture, including adversarial exclusions.Live supervisor finished-scout handoff persisting an open workflow follow-up and an approved Process change toFM_HOME/data/retro-workflow-followups.md.Direct review of the public/retro <ticket>row inREADME.mdas static inspection only; this was not a live scenario.✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.