Repository navigation
feat: add phase1 postmortem onboarding - #71
Merged
Merged
Conversation
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a Phase 1 postmortem onboarding pipeline: CLI command Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as "CLI\n(phase1 cmd)"
participant Phase1 as "postmortem_phase1\n(build_phase1_artifacts)"
participant Git as "Git\nrepo"
participant Parser as "Commit\nParser"
participant Clusterer as "Clusterer"
participant FS as "Filesystem"
User->>CLI: run `postmortem phase1 --repo . --output-dir postmortem`
CLI->>Phase1: build_phase1_artifacts(repo, output_dir)
Phase1->>Git: git log --name-only --format=...
Git-->>Phase1: commits + file lists
Phase1->>Parser: parse commit subjects → FixCommit[]
Parser-->>Phase1: FixCommit[]
Phase1->>Clusterer: group commits → PatternGroups
Clusterer-->>Phase1: PatternGroups
Phase1->>FS: atomically write README, taxonomy, index, onboarding, templates, patterns
FS-->>Phase1: write confirmations
Phase1-->>CLI: Phase1BuildResult {pattern_count, commit_count, output_dir}
CLI-->>User: print generated artifacts path and counts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds a new “Phase 1 postmortem onboarding” workflow to the coding_agent CLI that mines local git fix history and generates a versioned postmortem/ knowledge base (taxonomy, index, onboarding reports, templates, and initial pattern docs), along with regression tests and an ADR documenting the decision.
Changes:
- Add
coding_agent postmortem phase1CLI command and Phase 1 artifact generator (postmortem_phase1.py). - Add regression tests for commit collection, history scoping, clustering/grouping, and CLI behavior.
- Check in the generated
postmortem/corpus (templates, taxonomy/index, onboarding reports, and initial pattern documents).
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| coding-agent/src/coding_agent/postmortem_phase1.py | Implements git-history mining, clustering, and writing the Phase 1 postmortem artifact tree. |
| coding-agent/src/coding_agent/main.py | Adds postmortem phase1 CLI group/command and a small typing adjustment in stats. |
| coding-agent/tests/coding_agent/test_postmortem_phase1.py | Adds unit-style tests for fix-commit parsing, scoping, grouping, and artifact layout. |
| coding-agent/tests/cli/test_postmortem.py | Adds CLI test ensuring postmortem phase1 generates expected outputs. |
| coding-agent/docs/adr/0013-adopt-phase1-postmortem-onboarding.md | ADR documenting the Phase 1 onboarding approach and acceptance criteria. |
| coding-agent/.opencode/prompts/examples/postmortem-phase1-onboarding-task-packet.md | Task packet example describing scope, tests, and loop policy for this feature. |
| coding-agent/postmortem/README.md | Generated knowledge base README describing layout and Phase 1 scope. |
| coding-agent/postmortem/taxonomy.yaml | Generated taxonomy values used by Phase 1 output. |
| coding-agent/postmortem/index.yaml | Generated index of the initial pattern set (IDs, paths, subsystems, related files/commits, keywords). |
| coding-agent/postmortem/onboarding/ingestion-log.md | Generated onboarding log describing Phase 1 run summary. |
| coding-agent/postmortem/onboarding/historical-fix-clusters.md | Generated summary of clustered fix patterns (subsystem/commits/files). |
| coding-agent/postmortem/onboarding/commit-classification-report.md | Generated list of classified fix commits and inferred scopes. |
| coding-agent/postmortem/templates/pattern.md | Generated template for adding future pattern documents. |
| coding-agent/postmortem/templates/release-risk-report.md | Generated template for future release risk reports. |
| coding-agent/postmortem/patterns/PM-0001-address-code-review-issues.md | Generated initial pattern document. |
| coding-agent/postmortem/patterns/PM-0002-address-code-review-p0-p1-issues.md | Generated initial pattern document. |
| coding-agent/postmortem/patterns/PM-0003-guard-missing-directive-executors.md | Generated initial pattern document. |
| coding-agent/postmortem/patterns/PM-0004-handle-mapping-tool-results-safely.md | Generated initial pattern document. |
| coding-agent/postmortem/patterns/PM-0005-redact-user-facing-tool-result-displays.md | Generated initial pattern document. |
| coding-agent/postmortem/patterns/PM-0006-add-usage-event-fields-and-fix-tool-name-kwarg-in-pipeline.md | Generated initial pattern document. |
| coding-agent/postmortem/patterns/PM-0007-move-anchor-into-tape-models-to-remove-import-cycle.md | Generated initial pattern document. |
| coding-agent/postmortem/patterns/PM-0008-move-handoff-summaries-ahead-of-recent-context.md | Generated initial pattern document. |
| coding-agent/postmortem/patterns/PM-0009-preserve-neutral-bare-anchor-semantics.md | Generated initial pattern document. |
| coding-agent/postmortem/patterns/PM-0010-route-incremental-context-append-through-tapeview.md | Generated initial pattern document. |
| coding-agent/postmortem/patterns/PM-0011-centralize-session-approval-coordination.md | Generated initial pattern document. |
| coding-agent/postmortem/patterns/PM-0012-clean-up-pending-requests-after-waits.md | Generated initial pattern document. |
| coding-agent/postmortem/patterns/PM-0013-clear-answered-request-projections.md | Generated initial pattern document. |
| coding-agent/postmortem/patterns/PM-0014-make-approval-responses-single-shot.md | Generated initial pattern document. |
| coding-agent/postmortem/patterns/PM-0015-require-store-backed-requests-across-http-approval-flow.md | Generated initial pattern document. |
| coding-agent/postmortem/patterns/PM-0016-align-sandbox-env-error-assertion.md | Generated initial pattern document. |
| coding-agent/postmortem/patterns/PM-0017-preserve-shared-bootstrap-contracts.md | Generated initial pattern document. |
| coding-agent/postmortem/patterns/PM-0018-restore-child-pipeline-bootstrap-wiring.md | Generated initial pattern document. |
| coding-agent/postmortem/patterns/PM-0019-restore-child-pipeline-composition-root.md | Generated initial pattern document. |
| coding-agent/postmortem/patterns/PM-0020-checkpoint-ids-in-fs-store.md | Generated initial pattern document. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+306
to
+310
| for path in files: | ||
| lowered = path.lower() | ||
| if "ui" in lowered: | ||
| return "ui" | ||
| if "http" in lowered: |
There was a problem hiding this comment.
_infer_subsystem() is using naive substring checks like if "ui" in lowered, which will misclassify many non-UI paths (e.g., src/agentkit/context/builder.py contains the substring ui in builder). This will produce incorrect subsystem grouping whenever a fix commit has no explicit scope. Consider matching on path components (e.g., Path(path).parts) and/or well-scoped prefixes like "/ui/", "coding_agent/ui", etc., and avoid raw substring matches for short tokens like ui.
Comment on lines
+345
to
+379
| def _normalize_subsystem(subsystem: str) -> str: | ||
| aliases = { | ||
| "agentkit": "agentkit", | ||
| "adapter": "adapter", | ||
| "approval": "approval", | ||
| "bootstrap": "bootstrap", | ||
| "checkpoint": "checkpoint", | ||
| "cli": "cli", | ||
| "coding-agent": "runtime", | ||
| "config": "config", | ||
| "core": "runtime", | ||
| "docker": "runtime", | ||
| "gitignore": "runtime", | ||
| "http": "http", | ||
| "kb": "kb", | ||
| "p0": "runtime", | ||
| "p1": "runtime", | ||
| "plugins": "plugins", | ||
| "review": "ui", | ||
| "runtime": "runtime", | ||
| "session": "session_manager", | ||
| "session_manager": "session_manager", | ||
| "shell": "shell", | ||
| "skills": "plugins", | ||
| "storage": "storage_pg", | ||
| "tape": "tape", | ||
| "test": "verification", | ||
| "tools": "tools", | ||
| "tracing": "tracing", | ||
| "tui": "ui", | ||
| "ui": "ui", | ||
| "verification": "verification", | ||
| } | ||
| normalized = aliases.get(subsystem.lower(), subsystem.lower()) | ||
| if normalized not in TAXONOMY["subsystem"]: |
There was a problem hiding this comment.
_normalize_subsystem() has an alias for "coding-agent" but not "coding_agent" (underscore). The repo’s own history includes fix(coding_agent): ... subjects (see generated commit-classification report in this PR), which will currently normalize to runtime due to being treated as an unknown subsystem. Add an alias for coding_agent (and any other known scopes you expect, like store) so scoped commits don’t lose their intended classification.
Comment on lines
+463
to
+473
| def _build_ingestion_log( | ||
| repo_root: Path, output_dir: Path, commits: list[FixCommit] | ||
| ) -> str: | ||
| _ = repo_root | ||
| return "\n".join( | ||
| [ | ||
| "# Ingestion Log", | ||
| "", | ||
| "Repository: `.`", | ||
| f"Output directory: `{output_dir.name}`", | ||
| f"Collected fix commits: {len(commits)}", |
There was a problem hiding this comment.
_build_ingestion_log() always emits Repository: .``, even when the user runs the CLI with --repo pointing somewhere else. This makes the log misleading. To keep output deterministic without leaking absolute paths, consider recording a sanitized identifier (e.g., `repo_root.name`) or a path relative to the current working directory when it’s safe, instead of hardcoding `.`.
Comment on lines
+394
to
+405
| def _index_entry(pattern: PatternRecord, *, output_dir: Path) -> dict[str, object]: | ||
| _ = output_dir | ||
| return { | ||
| "id": pattern["id"], | ||
| "title": pattern["title"], | ||
| "path": str(Path("patterns") / pattern["filename"]), | ||
| "severity": pattern["severity"], | ||
| "confidence": pattern["confidence"], | ||
| "subsystems": pattern["subsystems"], | ||
| "related_files": pattern["related_files"], | ||
| "related_commits": pattern["related_commits"], | ||
| "keywords": pattern["keywords"], |
There was a problem hiding this comment.
_index_entry() takes output_dir but doesn’t use it (it’s immediately assigned to _). This makes the API look more complex than it is and can confuse future callers. Either remove the parameter entirely or use it to compute the returned path more explicitly (e.g., ensuring it’s relative to output_dir).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
coding-agent/postmortem/templates/pattern.md (1)
17-24: Template is missing core sections used by generated pattern docs.Line 17–24 omits “Trigger Conditions” and “Known Fix Signals,” which are present in the actual pattern artifacts. Adding them will keep manual and generated patterns consistent.
Suggested update
# Summary Short pattern summary. +# Trigger Conditions + +- Changes in <subsystem> paths +- Historical commit: `fix(scope): short description` + +# Known Fix Signals + +- `src/path/to/affected_file.py` + # Release Review Checklist - Review touched files. - Run related regression tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coding-agent/postmortem/templates/pattern.md` around lines 17 - 24, The template in pattern.md is missing the core sections "Trigger Conditions" and "Known Fix Signals" used by generated pattern docs; update the template to include these headings and placeholder content under the existing Summary and Release Review Checklist so the manual template matches generated artifacts—add a "Trigger Conditions" section describing when the pattern applies and a "Known Fix Signals" section listing indicators of successful remediation.coding-agent/postmortem/patterns/PM-0007-move-anchor-into-tape-models-to-remove-import-cycle.md (1)
7-13: Consider addingtapeto subsystem tags for better signal precision.Line 7–8 currently uses only
agentkit, but Line 12–13 are tape-specific paths. Addingtape(possibly alongsideagentkit) would make triggering/review routing more precise.Suggested update
subsystems: - agentkit +- tape🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coding-agent/postmortem/patterns/PM-0007-move-anchor-into-tape-models-to-remove-import-cycle.md` around lines 7 - 13, The front-matter currently lists only the subsystem "agentkit" under the subsystems key but the related_files are tape-specific; update the subsystems entry to include "tape" (e.g., change subsystems: - agentkit to subsystems: - agentkit - tape or replace with - tape if preferred) so review/trigger routing is more precise; edit the subsystems key in PM-0007-move-anchor-into-tape-models-to-remove-import-cycle.md to add the "tape" tag.coding-agent/postmortem/patterns/PM-0009-preserve-neutral-bare-anchor-semantics.md (1)
107-190: Reduce duplicated signal lists to avoid doc drift.
Known Fix Signalscurrently duplicates the longrelated_filesfrontmatter list. Consider making one canonical source (preferably frontmatter) and referencing it in the body to keep generated artifacts easier to maintain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coding-agent/postmortem/patterns/PM-0009-preserve-neutral-bare-anchor-semantics.md` around lines 107 - 190, The "Known Fix Signals" section duplicates the long related_files list; remove the in-body file list and instead reference the canonical frontmatter field (related_files) or a YAML/Markdown anchor to avoid duplication. Update the PM-0009-preserve-neutral-bare-anchor-semantics.md content so the header "Known Fix Signals" contains a short sentence like "See frontmatter related_files" (or an include of the frontmatter variable) and keep the full file enumeration only in the frontmatter (or a single shared anchor), ensuring readers still see the list but maintenance happens in one place.coding-agent/postmortem/taxonomy.yaml (1)
1-40: Consider eliminating taxonomy drift risk between code and artifact.
coding-agent/src/coding_agent/postmortem_phase1.pyalso hardcodes the taxonomy, so this checked-in file and runtime validation source can diverge. Prefer one source of truth (load from this YAML in the builder) or add a strict parity test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coding-agent/postmortem/taxonomy.yaml` around lines 1 - 40, The hardcoded taxonomy in coding-agent/src/coding_agent/postmortem_phase1.py can diverge from the checked-in taxonomy YAML (coding-agent/postmortem/taxonomy.yaml); replace the in-code hardcoded lists in postmortem_phase1.py with loading/parsing logic that reads the YAML at runtime (or during build) and use those loaded values (e.g., the variables or constants currently defined in postmortem_phase1.py) everywhere they are referenced, or alternatively add a strict parity unit test (e.g., test_taxonomy_parity) that loads both the YAML and the in-code taxonomy and fails if they differ to enforce a single source of truth.coding-agent/tests/coding_agent/test_postmortem_phase1.py (1)
1-11: Movesubprocessimport to module level.The
import subprocessstatement appears inline inside each test function (lines 19, 79, 190, 252, 326). While this works, it's more conventional to place imports at the module level for clarity and to avoid repeated import overhead.♻️ Suggested refactor
from __future__ import annotations from pathlib import Path +import subprocess import yaml from coding_agent.postmortem_phase1 import ( build_phase1_artifacts, collect_fix_commits, )Then remove the inline
import subprocessstatements from each test function.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coding-agent/tests/coding_agent/test_postmortem_phase1.py` around lines 1 - 11, Move the inline "import subprocess" statements to the top of the module by adding a single "import subprocess" at module level and remove the duplicate inline imports from each test function (the tests that call subprocess around lines 19, 79, 190, 252, 326). Update any test functions that currently perform "import subprocess" to rely on the module-level subprocess import; no functional changes otherwise. Ensure imports remain grouped with the existing standard-library imports alongside Path and yaml and keep build_phase1_artifacts and collect_fix_commits imports unchanged.coding-agent/src/coding_agent/postmortem_phase1.py (4)
394-406: Unusedoutput_dirparameter.The
output_dirparameter is accepted but immediately discarded with_ = output_dir. If this is reserved for future use (e.g., computing relative paths differently), consider adding a brief comment. Otherwise, it could be removed from the signature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coding-agent/src/coding_agent/postmortem_phase1.py` around lines 394 - 406, The output_dir parameter of _index_entry is unused; either remove output_dir from the function signature (and update any callers of _index_entry) or use it to build the returned "path" value (e.g., compute Path(output_dir) / "patterns" / pattern["filename"] and return its string or a path relative to output_dir), and if you intend to keep output_dir for future use add a short comment above _index_entry explaining why it is currently unused; update callers accordingly if you change the signature.
305-342: Consider dictionary-based subsystem inference.The
_infer_subsystemfunction uses a long if/elif chain with substring checks. This works correctly but could be more maintainable as a dictionary mapping patterns to subsystems.♻️ Optional: dictionary-based approach
def _infer_subsystem(files: list[str]) -> str: patterns = [ ("ui", "ui"), ("http", "http"), ("storage", "storage_pg"), ("pg", "storage_pg"), ("cli", "cli"), ("adapter", "adapter"), ("approval", "approval"), ("checkpoint", "checkpoint"), ("trace", "tracing"), ("tape", "tape"), ("tool", "tools"), ("shell", "shell"), ("plugin", "plugins"), ("config", "config"), ("kb", "kb"), ("session_manager", "session_manager"), ("session", "session_manager"), ("verify", "verification"), ("agentkit", "agentkit"), ] for path in files: lowered = path.lower() for pattern, subsystem in patterns: if pattern in lowered: return subsystem return "runtime"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coding-agent/src/coding_agent/postmortem_phase1.py` around lines 305 - 342, Replace the long if/elif substring chain in _infer_subsystem with a small ordered mapping (list of (pattern, subsystem) tuples or an OrderedDict) and iterate patterns for each path.lower() to return the first matching subsystem; ensure more specific keys like "session_manager" come before "session" and include both "storage" and "pg" mapping to "storage_pg" so behavior remains identical to the original function.
463-478: Unusedrepo_rootparameter in_build_ingestion_log.Similar to
_index_entry, therepo_rootparameter is discarded. The log hardcodes"Repository: \.\""rather than using the actual repo path. This appears intentional (to avoid leaking absolute paths as tested), but the parameter creates a misleading signature. Consider removing it or adding a comment explaining the intentional hardcoding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coding-agent/src/coding_agent/postmortem_phase1.py` around lines 463 - 478, The _build_ingestion_log function currently has an unused repo_root parameter and hardcodes "Repository: `.`" which is intentional to avoid leaking absolute paths; either remove repo_root from the function signature and all call sites (and update any type hints/imports) or keep the parameter but add a clear comment above _build_ingestion_log stating that repo_root is intentionally unused and the repository is deliberately rendered as "." to avoid leaking absolute paths—pick one approach and apply it consistently (if removing, update callers that pass repo_root; if keeping, document with a TODO or explanatory comment referencing _build_ingestion_log).
21-28: Mutablelistfield in frozen dataclass.The
filesfield is a mutablelist[str]inside a frozen dataclass. While the dataclass itself is immutable (can't reassigncommit.files = new_list), the list contents can still be mutated viacommit.files.append(...)orcommit.files.clear().If true immutability is desired, consider using
tuple[str, ...]instead. However, since the current usage doesn't appear to mutate the list after creation, this is a minor concern.♻️ Optional: use tuple for full immutability
`@dataclass`(frozen=True) class FixCommit: sha: str subject: str scope: str | None summary: str - files: list[str] + files: tuple[str, ...]This would require updating the call site at line 144 to pass
tuple(files)instead offiles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coding-agent/src/coding_agent/postmortem_phase1.py` around lines 21 - 28, FixCommit currently declares files: list[str] inside a frozen dataclass which allows mutating the list contents; change the field to files: tuple[str, ...] to enforce immutability (update the annotation in class FixCommit) and update all construction sites where FixCommit is created to wrap lists with tuple(files) or pass a tuple literal so callers produce a tuple instead of a list; ensure any downstream code that expects list methods is adapted to use tuple or convert via list(...) where mutation is needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@coding-agent/docs/adr/0013-adopt-phase1-postmortem-onboarding.md`:
- Around line 3-4: Update the ADR header's Status field from "Proposed" to
reflect that the decision has been adopted/implemented (e.g., change "Status:
Proposed" to "Status: Adopted" or "Status: Implemented") in the ADR file
0013-adopt-phase1-postmortem-onboarding.md; also adjust the Date field if you
want the implementation date recorded and ensure the header remains consistent
with the ADR title and body.
In `@coding-agent/postmortem/patterns/PM-0020-checkpoint-ids-in-fs-store.md`:
- Around line 1-21: The frontmatter title "Checkpoint ids in fs store" and the
summary "validate checkpoint ids in fs store" are inconsistent; update the title
to match the clearer, verb-based summary (e.g., change the title value to
"Validate checkpoint ids in fs store") or alternatively make the summary match
the existing title so both read the same; edit the frontmatter fields for id:
PM-0020 and the title key in the document to ensure they are aligned with the
summary.
In `@coding-agent/tests/cli/test_postmortem.py`:
- Around line 16-61: Replace repeated hardcoded "git" strings by resolving the
git executable once at the top of the test using shutil.which and store it in a
variable (e.g., git_exe); if shutil.which returns None, call pytest.skip to
gracefully skip the test. Then update every subprocess.run invocation in this
test (the calls that use repo, session_dir, session_file and run git
init/config/add/commit) to use the git_exe variable instead of the literal
"git"; also add the necessary imports for shutil and pytest at the top of the
test file.
---
Nitpick comments:
In
`@coding-agent/postmortem/patterns/PM-0007-move-anchor-into-tape-models-to-remove-import-cycle.md`:
- Around line 7-13: The front-matter currently lists only the subsystem
"agentkit" under the subsystems key but the related_files are tape-specific;
update the subsystems entry to include "tape" (e.g., change subsystems: -
agentkit to subsystems: - agentkit - tape or replace with - tape if preferred)
so review/trigger routing is more precise; edit the subsystems key in
PM-0007-move-anchor-into-tape-models-to-remove-import-cycle.md to add the "tape"
tag.
In
`@coding-agent/postmortem/patterns/PM-0009-preserve-neutral-bare-anchor-semantics.md`:
- Around line 107-190: The "Known Fix Signals" section duplicates the long
related_files list; remove the in-body file list and instead reference the
canonical frontmatter field (related_files) or a YAML/Markdown anchor to avoid
duplication. Update the PM-0009-preserve-neutral-bare-anchor-semantics.md
content so the header "Known Fix Signals" contains a short sentence like "See
frontmatter related_files" (or an include of the frontmatter variable) and keep
the full file enumeration only in the frontmatter (or a single shared anchor),
ensuring readers still see the list but maintenance happens in one place.
In `@coding-agent/postmortem/taxonomy.yaml`:
- Around line 1-40: The hardcoded taxonomy in
coding-agent/src/coding_agent/postmortem_phase1.py can diverge from the
checked-in taxonomy YAML (coding-agent/postmortem/taxonomy.yaml); replace the
in-code hardcoded lists in postmortem_phase1.py with loading/parsing logic that
reads the YAML at runtime (or during build) and use those loaded values (e.g.,
the variables or constants currently defined in postmortem_phase1.py) everywhere
they are referenced, or alternatively add a strict parity unit test (e.g.,
test_taxonomy_parity) that loads both the YAML and the in-code taxonomy and
fails if they differ to enforce a single source of truth.
In `@coding-agent/postmortem/templates/pattern.md`:
- Around line 17-24: The template in pattern.md is missing the core sections
"Trigger Conditions" and "Known Fix Signals" used by generated pattern docs;
update the template to include these headings and placeholder content under the
existing Summary and Release Review Checklist so the manual template matches
generated artifacts—add a "Trigger Conditions" section describing when the
pattern applies and a "Known Fix Signals" section listing indicators of
successful remediation.
In `@coding-agent/src/coding_agent/postmortem_phase1.py`:
- Around line 394-406: The output_dir parameter of _index_entry is unused;
either remove output_dir from the function signature (and update any callers of
_index_entry) or use it to build the returned "path" value (e.g., compute
Path(output_dir) / "patterns" / pattern["filename"] and return its string or a
path relative to output_dir), and if you intend to keep output_dir for future
use add a short comment above _index_entry explaining why it is currently
unused; update callers accordingly if you change the signature.
- Around line 305-342: Replace the long if/elif substring chain in
_infer_subsystem with a small ordered mapping (list of (pattern, subsystem)
tuples or an OrderedDict) and iterate patterns for each path.lower() to return
the first matching subsystem; ensure more specific keys like "session_manager"
come before "session" and include both "storage" and "pg" mapping to
"storage_pg" so behavior remains identical to the original function.
- Around line 463-478: The _build_ingestion_log function currently has an unused
repo_root parameter and hardcodes "Repository: `.`" which is intentional to
avoid leaking absolute paths; either remove repo_root from the function
signature and all call sites (and update any type hints/imports) or keep the
parameter but add a clear comment above _build_ingestion_log stating that
repo_root is intentionally unused and the repository is deliberately rendered as
"." to avoid leaking absolute paths—pick one approach and apply it consistently
(if removing, update callers that pass repo_root; if keeping, document with a
TODO or explanatory comment referencing _build_ingestion_log).
- Around line 21-28: FixCommit currently declares files: list[str] inside a
frozen dataclass which allows mutating the list contents; change the field to
files: tuple[str, ...] to enforce immutability (update the annotation in class
FixCommit) and update all construction sites where FixCommit is created to wrap
lists with tuple(files) or pass a tuple literal so callers produce a tuple
instead of a list; ensure any downstream code that expects list methods is
adapted to use tuple or convert via list(...) where mutation is needed.
In `@coding-agent/tests/coding_agent/test_postmortem_phase1.py`:
- Around line 1-11: Move the inline "import subprocess" statements to the top of
the module by adding a single "import subprocess" at module level and remove the
duplicate inline imports from each test function (the tests that call subprocess
around lines 19, 79, 190, 252, 326). Update any test functions that currently
perform "import subprocess" to rely on the module-level subprocess import; no
functional changes otherwise. Ensure imports remain grouped with the existing
standard-library imports alongside Path and yaml and keep build_phase1_artifacts
and collect_fix_commits imports unchanged.
🪄 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: 442373b8-2c4b-4521-b24e-d209b58e09ac
📒 Files selected for processing (34)
coding-agent/.opencode/prompts/examples/postmortem-phase1-onboarding-task-packet.mdcoding-agent/docs/adr/0013-adopt-phase1-postmortem-onboarding.mdcoding-agent/postmortem/README.mdcoding-agent/postmortem/index.yamlcoding-agent/postmortem/onboarding/commit-classification-report.mdcoding-agent/postmortem/onboarding/historical-fix-clusters.mdcoding-agent/postmortem/onboarding/ingestion-log.mdcoding-agent/postmortem/patterns/PM-0001-address-code-review-issues.mdcoding-agent/postmortem/patterns/PM-0002-address-code-review-p0-p1-issues.mdcoding-agent/postmortem/patterns/PM-0003-guard-missing-directive-executors.mdcoding-agent/postmortem/patterns/PM-0004-handle-mapping-tool-results-safely.mdcoding-agent/postmortem/patterns/PM-0005-redact-user-facing-tool-result-displays.mdcoding-agent/postmortem/patterns/PM-0006-add-usage-event-fields-and-fix-tool-name-kwarg-in-pipeline.mdcoding-agent/postmortem/patterns/PM-0007-move-anchor-into-tape-models-to-remove-import-cycle.mdcoding-agent/postmortem/patterns/PM-0008-move-handoff-summaries-ahead-of-recent-context.mdcoding-agent/postmortem/patterns/PM-0009-preserve-neutral-bare-anchor-semantics.mdcoding-agent/postmortem/patterns/PM-0010-route-incremental-context-append-through-tapeview.mdcoding-agent/postmortem/patterns/PM-0011-centralize-session-approval-coordination.mdcoding-agent/postmortem/patterns/PM-0012-clean-up-pending-requests-after-waits.mdcoding-agent/postmortem/patterns/PM-0013-clear-answered-request-projections.mdcoding-agent/postmortem/patterns/PM-0014-make-approval-responses-single-shot.mdcoding-agent/postmortem/patterns/PM-0015-require-store-backed-requests-across-http-approval-flow.mdcoding-agent/postmortem/patterns/PM-0016-align-sandbox-env-error-assertion.mdcoding-agent/postmortem/patterns/PM-0017-preserve-shared-bootstrap-contracts.mdcoding-agent/postmortem/patterns/PM-0018-restore-child-pipeline-bootstrap-wiring.mdcoding-agent/postmortem/patterns/PM-0019-restore-child-pipeline-composition-root.mdcoding-agent/postmortem/patterns/PM-0020-checkpoint-ids-in-fs-store.mdcoding-agent/postmortem/taxonomy.yamlcoding-agent/postmortem/templates/pattern.mdcoding-agent/postmortem/templates/release-risk-report.mdcoding-agent/src/coding_agent/__main__.pycoding-agent/src/coding_agent/postmortem_phase1.pycoding-agent/tests/cli/test_postmortem.pycoding-agent/tests/coding_agent/test_postmortem_phase1.py
Comment on lines
+1
to
+21
| --- | ||
| id: PM-0020 | ||
| title: Checkpoint ids in fs store | ||
| status: active | ||
| severity: medium | ||
| confidence: medium | ||
| subsystems: | ||
| - checkpoint | ||
| related_commits: | ||
| - 04252bf694892e8bce237f8e03cfff5077d41689 | ||
| related_files: | ||
| - src/agentkit/storage/checkpoint_fs.py | ||
| - tests/agentkit/checkpoint/test_service.py | ||
| release_checks: | ||
| - Run focused tests for checkpoint changes before release. | ||
| - Review affected files for the same control-flow shape before shipping. | ||
| --- | ||
|
|
||
| # Summary | ||
|
|
||
| validate checkpoint ids in fs store |
Contributor
There was a problem hiding this comment.
Title and summary mismatch.
Line 3 specifies the title as "Checkpoint ids in fs store" while Line 21 provides the summary as "validate checkpoint ids in fs store". The summary includes the verb "validate" which makes the intent clearer. Consider aligning the title with the summary for consistency.
📝 Proposed fix
---
id: PM-0020
-title: Checkpoint ids in fs store
+title: Validate checkpoint ids in fs store
status: active📝 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
| --- | |
| id: PM-0020 | |
| title: Checkpoint ids in fs store | |
| status: active | |
| severity: medium | |
| confidence: medium | |
| subsystems: | |
| - checkpoint | |
| related_commits: | |
| - 04252bf694892e8bce237f8e03cfff5077d41689 | |
| related_files: | |
| - src/agentkit/storage/checkpoint_fs.py | |
| - tests/agentkit/checkpoint/test_service.py | |
| release_checks: | |
| - Run focused tests for checkpoint changes before release. | |
| - Review affected files for the same control-flow shape before shipping. | |
| --- | |
| # Summary | |
| validate checkpoint ids in fs store | |
| --- | |
| id: PM-0020 | |
| title: Validate checkpoint ids in fs store | |
| status: active | |
| severity: medium | |
| confidence: medium | |
| subsystems: | |
| - checkpoint | |
| related_commits: | |
| - 04252bf694892e8bce237f8e03cfff5077d41689 | |
| related_files: | |
| - src/agentkit/storage/checkpoint_fs.py | |
| - tests/agentkit/checkpoint/test_service.py | |
| release_checks: | |
| - Run focused tests for checkpoint changes before release. | |
| - Review affected files for the same control-flow shape before shipping. | |
| --- | |
| # Summary | |
| validate checkpoint ids in fs store |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@coding-agent/postmortem/patterns/PM-0020-checkpoint-ids-in-fs-store.md`
around lines 1 - 21, The frontmatter title "Checkpoint ids in fs store" and the
summary "validate checkpoint ids in fs store" are inconsistent; update the title
to match the clearer, verb-based summary (e.g., change the title value to
"Validate checkpoint ids in fs store") or alternatively make the summary match
the existing title so both read the same; edit the frontmatter fields for id:
PM-0020 and the title key in the document to ensure they are aligned with the
summary.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
coding-agent/tests/coding_agent/test_postmortem_phase1.py (1)
21-37:⚠️ Potential issue | 🟠 MajorResolve repeated literal
gitsubprocess calls (S607) with one executable lookup.Lines in each test still invoke
"git"directly (e.g., Line 22, Line 82, Line 193, Line 255, Line 329). This will keep Ruff S607 failing and makes tests brittle whengitis missing.🔧 Suggested pattern (apply across all tests in this file)
+import shutil +import pytest import yaml from coding_agent.postmortem_phase1 import ( build_phase1_artifacts, collect_fix_commits, ) +def _git_bin() -> str: + git_bin = shutil.which("git") + if git_bin is None: + pytest.skip("git executable not available in PATH") + return git_bin + def test_collect_fix_commits_parses_subjects_and_files_from_git_history( tmp_path: Path, ) -> None: + git_bin = _git_bin() repo = tmp_path / "repo" repo.mkdir() @@ - subprocess.run(["git", "init"], cwd=repo, check=True, capture_output=True, text=True) + subprocess.run([git_bin, "init"], cwd=repo, check=True, capture_output=True, text=True)Also applies to: 81-97, 192-208, 254-270, 328-344
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coding-agent/tests/coding_agent/test_postmortem_phase1.py` around lines 21 - 37, The test repeatedly invokes the literal "git" in subprocess.run calls which triggers Ruff S607; locate the git subprocess invocations in coding_agent/tests/coding_agent/test_postmortem_phase1.py (the subprocess.run calls shown) and perform a one-time executable lookup using shutil.which (e.g., assign to git_exe) at the top of the test module or fixture, then replace all literal "git" arguments in subprocess.run calls with the git_exe variable (preserving existing args, cwd, check, capture_output, text parameters) so tests use the discovered executable and avoid repeated literal lookups.coding-agent/src/coding_agent/postmortem_phase1.py (1)
106-121:⚠️ Potential issue | 🟠 MajorUse a resolved absolute git executable in both subprocess call paths.
At Line 106 and Line 276, invoking
"git"via partial path leaves a Ruff S607 blocker insrc/and weakens execution safety. Resolve once viashutil.which("git")and reuse it for all git subprocess calls.As per coding guidelines, `coding-agent/src/**/*.py` should remain Ruff-clean (`uv run ruff format src/`).🔧 Proposed fix
import os import re +import shutil import subprocess import tempfile @@ +def _resolve_git_bin() -> str: + git_bin = shutil.which("git") + if git_bin is None: + raise RuntimeError("git executable not available in PATH") + return git_bin + def collect_fix_commits(repo_root: Path) -> list[FixCommit]: + git_bin = _resolve_git_bin() git_root = _git_toplevel(repo_root) @@ - command = [ - "git", + command = [ + git_bin, "log", @@ def _git_toplevel(repo_root: Path) -> Path: + git_bin = _resolve_git_bin() completed = subprocess.run( - ["git", "rev-parse", "--show-toplevel"], + [git_bin, "rev-parse", "--show-toplevel"], check=True, capture_output=True, text=True, cwd=repo_root, )Also applies to: 275-281
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coding-agent/src/coding_agent/postmortem_phase1.py` around lines 106 - 121, Replace literal "git" invocations with a resolved absolute path: call shutil.which("git") once at module or function scope (e.g., at top of postmortem_phase1.py or inside the function that builds the `command` list) and store it in a variable like `git_executable`; then use that variable instead of the string "git" when constructing `command` and in all other subprocess.run calls in this module (including the command built at the `command = ["git", "log", ...]` site and the later invocation around lines ~275-281). Also add a guard that raises a clear error if shutil.which returns None. Ensure both call sites (the `command` list at Line 106 and the later git subprocess at Line 276) use the same `git_executable` to satisfy Ruff S607.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@coding-agent/src/coding_agent/postmortem_phase1.py`:
- Around line 106-121: Replace literal "git" invocations with a resolved
absolute path: call shutil.which("git") once at module or function scope (e.g.,
at top of postmortem_phase1.py or inside the function that builds the `command`
list) and store it in a variable like `git_executable`; then use that variable
instead of the string "git" when constructing `command` and in all other
subprocess.run calls in this module (including the command built at the `command
= ["git", "log", ...]` site and the later invocation around lines ~275-281).
Also add a guard that raises a clear error if shutil.which returns None. Ensure
both call sites (the `command` list at Line 106 and the later git subprocess at
Line 276) use the same `git_executable` to satisfy Ruff S607.
In `@coding-agent/tests/coding_agent/test_postmortem_phase1.py`:
- Around line 21-37: The test repeatedly invokes the literal "git" in
subprocess.run calls which triggers Ruff S607; locate the git subprocess
invocations in coding_agent/tests/coding_agent/test_postmortem_phase1.py (the
subprocess.run calls shown) and perform a one-time executable lookup using
shutil.which (e.g., assign to git_exe) at the top of the test module or fixture,
then replace all literal "git" arguments in subprocess.run calls with the
git_exe variable (preserving existing args, cwd, check, capture_output, text
parameters) so tests use the discovered executable and avoid repeated literal
lookups.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f35cab47-2308-4f95-b0eb-43fa4bb62f22
📒 Files selected for processing (4)
coding-agent/docs/adr/0013-adopt-phase1-postmortem-onboarding.mdcoding-agent/src/coding_agent/postmortem_phase1.pycoding-agent/tests/cli/test_postmortem.pycoding-agent/tests/coding_agent/test_postmortem_phase1.py
✅ Files skipped from review due to trivial changes (1)
- coding-agent/docs/adr/0013-adopt-phase1-postmortem-onboarding.md
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
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
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
./postmortem/knowledge baseTest Plan
uv run pytest tests/coding_agent/test_postmortem_phase1.py tests/cli/test_postmortem.py -vuv run pytest tests/cli/ -vuv run python -m coding_agent postmortem phase1 --repo . --output-dir postmortemSummary by CodeRabbit
New Features
Documentation
Tests