feat(doghouse): the reboot — PR flight recorder engine#5
Open
flyingrobots wants to merge 50 commits intomainfrom
Open
feat(doghouse): the reboot — PR flight recorder engine#5flyingrobots wants to merge 50 commits intomainfrom
flyingrobots wants to merge 50 commits intomainfrom
Conversation
… --version and review --format-list
…ent with non-JSON handling
…ude/gemini/other)
…n; llm builder reads from config when env absent (failing)
…cmd when env missing
…nd flatten_review_threads service
…--enable-anna flag for now; tests for adapter and config write
…abbit_only; tests; update sample config
…I with secret code; wire cli tui command; tests for gh adapter
…; repo util for owner/repo; wire PR selection to comments
…adapter implements list_open_prs; author wired
…prs; comments viewer logs counts; gh adapter author+list_open_prs
…progress callback logs
…; TUI option to apply suggestion with auto commit; extend GitPort for add_and_commit; sample config reply flag
…d; tests; GitPort head_sha
…estion, replies, summary/help/batch send)
… gh CLI; TUI uses selector
…d docs/mind: SPEC, TECH-SPEC (with Mermaid), SPRINTS, FEATURES, TASKLIST, DRIFT_REPORT\n- Add git_mind hex skeleton: ports/adapters/services wrapping Draft Punks\n- Add plumbing snapshot engine writing commits under refs/mind/sessions/* with DP-* trailers\n- Add CLI: git mind (aliases pending): session-new, state-show, repo-detect, pr-list, pr-pick, nuke\n- Add JSONL stdio server: hello, state.show, repo.detect, pr.list, pr.select (CAS via expect_state)\n- Add initial tests (temp repo snapshot round-trip)\n- Seed PRODUCTION_LOG.mg entry for CLI pivot\n\nThis lays the foundation for the GATOS idea (Git Attested, Transactional Operating Surface).
…ompatibility (add_options fallback)\n- CommentViewer/PRPicker as Screens; compose/mount fixes\n- ASCII banner and title layout\n- Makefile: dev-venv + install-dev + run/tui targets\n- Workflows: seed CI/publish (skeleton)
…ootstrap-git-mind.sh to export git_mind sources/docs/tests into a fresh repo (new history)\n- Add make bootstrap-git-mind DEST=~/git-mind target
…(git-native operating surface)
…T, job graph, caps, mind remotes, artifacts, kernels, RMG
…ow) and Debug LLM (success/fail)
…success/fail), state caching and selection, CAS guarded
…nv gap in PRODUCTION_LOG
…L, pub/sub, hot cache, prototype CLI
… phases, crosswalk, risks, next steps
…\n- Delete legacy Draft Punks TUI and git-mind kernel\n- Implement core doghouse package (domain, services, ports, adapters)\n- Add snapshot, history, and playback CLI commands\n- Add machine-readable --json output for snapshot\n- Update README and Makefile for doghouse focus
…PhiedBach / Snoopy narrative in README\n- Add PhiedBach persona to CLI snapshot/playback output\n- Update TASKLIST.md to reflect Doghouse reboot progress
…Bach's sliding spectacles, Red Bull hissing, and BunBun's basket-gnawing humor throughout the README.
…- Add 'watch' command for live monitoring\n- Add 'export' command for black box repro bundles\n- Implement GitAdapter for local dirty state detection\n- Implement Blocking Matrix (Primary/Secondary) in GhCliAdapter\n- Archive legacy docs to docs/archive/
Contributor
Summary by CodeRabbit
WalkthroughThis PR introduces Doghouse 2.0, a PR flight-recorder system that tracks review state changes across pushes and checks. It adds domain models ( Changes
Sequence Diagram(s)sequenceDiagram
participant User as User / CLI
participant CLI as doghouse snapshot
participant RecorderService as RecorderService
participant GitHubAdapter as GhCliAdapter
participant GitAdapter as GitAdapter
participant DeltaEngine as DeltaEngine
participant StorageAdapter as JSONLStorageAdapter
User->>CLI: doghouse snapshot --repo owner/repo --pr 42
CLI->>RecorderService: record_sortie(repo, pr_id)
par Fetch Remote State
RecorderService->>GitHubAdapter: get_head_sha(pr_id)
GitHubAdapter-->>RecorderService: current_sha
RecorderService->>GitHubAdapter: fetch_blockers(pr_id)
GitHubAdapter-->>RecorderService: remote_blockers
and Fetch Local State
RecorderService->>GitAdapter: get_local_blockers()
GitAdapter-->>RecorderService: local_blockers
end
RecorderService->>RecorderService: merge_blockers(remote, local)
RecorderService->>RecorderService: build_snapshot(merged_blockers, current_sha)
RecorderService->>StorageAdapter: get_latest_snapshot(repo, pr_id)
StorageAdapter-->>RecorderService: baseline_snapshot (or None)
RecorderService->>DeltaEngine: compute_delta(baseline, current)
DeltaEngine-->>RecorderService: delta
RecorderService->>StorageAdapter: save_snapshot(repo, pr_id, current)
RecorderService-->>CLI: (Snapshot, Delta)
CLI->>CLI: format_output(snapshot, delta)
CLI-->>User: blockers table + verdict
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Reasoning: This PR introduces heterogeneous changes spanning domain models, adapter implementations with subprocess/GitHub API interaction, service orchestration with blocker-merging logic, CLI multi-command routing with auto-detection, storage persistence, and extensive documentation. While many individual documentation files are low-effort, the core application logic requires careful validation of: (1) GitHub adapter blocker parsing and error paths; (2) recursive blocker ID merging strategy and severity conflict resolution; (3) Delta computation determinism and verdict logic; (4) CLI command robustness around missing upstream/repo detection; and (5) fixture coverage. The variety of concerns (I/O, state management, API contracts) and interaction patterns between services demand separate reasoning per cohort rather than pattern-matching a single refactor. Possibly related issues
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f339fbcf8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| authors = [{name = "Draft Punks"}] | ||
| requires-python = ">=3.11" | ||
| dependencies = ["typer>=0.12", "rich>=13.7", "textual>=0.44", "requests>=2.31"] | ||
| readme = { file = "cli/README.md", content-type = "text/markdown" } |
There was a problem hiding this comment.
pyproject.toml declares readme = { file = "cli/README.md" }, but this commit only adds README.md and doghouse/README.md; there is no cli/README.md in the tree. Builds that read package metadata (including the new publish workflow’s python -m build) will fail when they try to load the missing README file, blocking packaging and release.
Useful? React with 👍 / 👎.
src/doghouse/cli/main.py
Outdated
| repo = repo or detected_repo | ||
| pr = pr or detected_pr | ||
|
|
||
| github = GhCliAdapter() |
There was a problem hiding this comment.
The CLI accepts a --repo option, but snapshot creates GhCliAdapter() with no repo context, so _run_gh never adds -R owner/name and all PR data is fetched from the current checkout instead of the requested repo. If a user runs from one repository while passing --repo for another, snapshots are written under the requested repo key but contain data from a different repository, producing incorrect history and deltas.
Useful? React with 👍 / 👎.
| snapshot, delta = service.record_sortie(repo, pr) | ||
|
|
||
| # Only announce if something changed or it's the first run | ||
| if delta.baseline_sha or delta.added_blockers or delta.removed_blockers: |
There was a problem hiding this comment.
This gate is intended to print only on change or first run, but delta.baseline_sha becomes truthy after the first saved snapshot and stays truthy forever, so the block executes every interval even when there are no blocker or SHA changes. Long-running watch sessions will continuously spam output/logs instead of signaling only meaningful transitions.
Useful? React with 👍 / 👎.
![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: 50
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
1-21:⚠️ Potential issue | 🟠 MajorSet explicit
GITHUB_TOKENpermissions (still missing).The workflow still omits an explicit
permissionsblock; default token scope remains broader than needed.🔒 Minimal least-privilege baseline
name: CI +'permissions': + contents: read 'on': push: branches: [main]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 1 - 21, The workflow named "CI" (jobs.test) lacks an explicit top-level permissions block; add a top-level permissions mapping to restrict the GITHUB_TOKEN to least-privilege (for example set contents: read and pull-requests: write or other minimal scopes your tests/actions need) so the token scope is explicit and limited; place the permissions block directly under the workflow header (near "name: CI") so jobs use the constrained token.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 2-6: The workflow trigger currently restricts CI to only the tui
branch by specifying branches: [ tui ] under both push and pull_request; update
the on -> push and on -> pull_request branch filters (the branches arrays) to
include main (or remove the branch restriction to run on all branches) so PRs
and pushes to main are validated; locate the on/push and on/pull_request
sections in the CI file and modify the branches arrays accordingly.
In @.github/workflows/publish.yml:
- Around line 9-10: Update the workflow permissions block to include id-token:
write alongside contents: read and switch the publish step to use OIDC Trusted
Publishing: add "id-token: write" under the existing permissions (keeping
"contents: read"), then remove the legacy "password" input from the publish
job/step and use the OIDC-based authentication approach for PyPI publishing
(adjust the publish step that currently uses the password input to the OIDC
token flow).
- Around line 6-23: The current single job "build-and-publish" runs both the
Build and Publish steps, which risks tampering between build and publish; split
this into two jobs (e.g., "build" and "publish") where the build job runs the
Build step (pin the build tool like "python -m pip install --upgrade pip
build==<version>" and be explicit about --no-build-isolation if used), saves the
resulting artifacts using actions/upload-artifact, and the publish job
(depends-on the build job) downloads the artifact with actions/download-artifact
and then runs the pypa/gh-action-pypi-publish step to publish; also ensure the
Publish job uses a fixed action version for pypa/gh-action-pypi-publish and
retains the existing secrets usage for password.
- Around line 4-5: Replace the overly permissive tags glob under the tags key
that currently reads 'v*.*.*' with a stricter validation: either change the
pattern to a numeric-only form (e.g., use a regex-like pattern such as
'v[0-9]+\\.[0-9]+\\.[0-9]+' for systems that accept regex) or add a prerelease
validation step that checks the pushed tag matches /^\v[0-9]+\.[0-9]+\.[0-9]+$/
before proceeding; update the tags entry (the line containing "tags: -
'v*.*.*'") or add the validation job referencing the same tags key so only
semantic-version tags are accepted.
In `@docs/archive/DRIFT_REPORT.md`:
- Around line 59-74: The ordered list in the DRIFT_REPORT.md section containing
items "DP-F-10 Prompt Editing & Templates" through "DP-F-16 Theming & Layout"
uses explicit numbered prefixes like "11)"–"16)" which violates MD029; update
that block so each list item uses the configured normalized numbering (e.g.
start each line with "1." or remove explicit numbers to allow automatic
numbering) for the entries referencing DP-F-10, DP-F-11, DP-F-12, DP-F-13,
DP-F-15, and DP-F-16 so markdownlint accepts the 1/1/1 style.
In `@docs/archive/IDEAS.md`:
- Around line 25-57: Multiple section headings (e.g., "3) Consensus & Grants",
"4) CRDT Mode (optional)", "5) Deterministic Job Graph", etc.) lack the required
blank line before and/or after them causing MD022 warnings; update the markdown
by ensuring each top-level heading in this block has a blank line above and
below the heading (insert one empty line before and one empty line after each
heading title) so headings like "3) Consensus & Grants", "4) CRDT Mode
(optional)", "5) Deterministic Job Graph", "6) Capability Tokens", "7) Mind
Remotes & Selective Replication", "8) Artifacts Store", and "9) Kernel Backends"
conform to markdownlint rules.
In `@docs/archive/INTEGRATIONS-git-kv.md`:
- Around line 25-57: Several headings in the provided markdown (e.g., "Phase 0 —
Adapter & Protocol", "Phase 1 — Index & TTL Alignment", "Phase 2 — Chunked
Values & Artifacts", "Phase 3 — Gateway & Remotes", "Phase 4 — Observability &
Watchers", "Open Questions", "Risks & Mitigations", "Next Steps") are missing
the required blank lines before/after them; add a single blank line above each
top-level heading and a single blank line after each heading (and before the
following paragraph or list) to satisfy MD022 and normalize spacing throughout
the document.
In `@docs/archive/mind/FEATURES.md`:
- Around line 8-85: Fix MD022 spacing by ensuring a single blank line before and
after each Markdown heading in this file; specifically adjust headings like
"GM-F-00 Snapshot Engine & JSONL", "GM-US-0001 Snapshot commits under
refs/mind/sessions/*", "GM-US-0002 JSONL serve --stdio (hello, state.show,
repo.detect, pr.list, pr.select)", "GM-F-01 PR & Threads", and all subheadings
(e.g., "User Story", "Requirements", "Acceptance", "DoR", "Test Plan") so they
have one blank line above and one blank line below, then run the markdown linter
to confirm MD022 is resolved across the document.
In `@docs/archive/mind/SPEC.md`:
- Around line 3-70: The file violates MD022 (missing blank lines around
headings); fix by ensuring a single blank line both before and after each
top-level and secondary heading (e.g., "## Vision", "## User Outcomes", "## Core
Flows (v0.1)", "## Non‑Goals (v0.1)", "## Reference Namespace (in‑repo; no
worktree churn)", "## CLI (human)", "## JSONL API (machine)", "## Privacy &
Artifacts (hybrid by default)", "## Policy & Attributes", "## Remotes", "##
Integrations") so every heading is separated from surrounding paragraphs and
lists with one blank line, normalize any headings that currently lack that
spacing, and run the markdown linter to verify MD022 is resolved.
In `@docs/archive/mind/TASKLIST.md`:
- Around line 5-36: Add a single blank line after each level-2 heading to
satisfy MD022: insert one empty line after "## GM-F-00 Snapshot & JSONL", "##
GM-F-01 PR & Threads", "## GM-F-02 LLM Debug & Real Template", "## GM-F-03
Artifacts & Remotes", and "## GM-F-04 Locks & Consensus" so the following list
items are separated from the headings; no other changes needed.
In `@docs/archive/mind/TECH-SPEC.md`:
- Around line 3-81: The file violates markdownlint rules MD022/MD031 due to
extra blank lines around headings and fenced blocks; fix by normalizing spacing
so there are no blank lines immediately before or after ATX headings like "## 1)
Architecture (Hexagonal)" and no blank lines directly inside or immediately
surrounding fenced code blocks (triple backticks) such as the Mermaid blocks;
update the sections containing "Mermaid — System Context" and "Mermaid — Commit
Flow" and all other headings to remove the offending blank lines so headings and
fences adhere to MD022/MD031.
In `@docs/archive/SPEC.md`:
- Around line 5-1166: The SPEC.md has pervasive markdownlint issues: missing
fence languages (MD040) on many fenced blocks (e.g., the triple-backtick blocks
under headings like "# 0. Scroll View Widget", "## UX Flow Diagram" mermaid
blocks, and the config example under "## Config Structure"), trailing
spaces/newline issues (MD009) throughout the doc, and a missing final newline
(MD047). Fix by adding appropriate fence languages (e.g., ```text for plain
screenshots/layout, ```mermaid for diagrams, ```toml for config blocks), remove
all trailing whitespace across the file (trim end-of-line spaces), and ensure
the file ends with a single newline; run markdownlint (or your repo lint task)
to verify no MD040/MD009/MD047 warnings remain.
In `@docs/code-reviews/PR5/56964e6b72bbe7639f9c725c6e9f2327f75bb402.md`:
- Line 30: The archived review artifact contains unresolved template
placeholders "{response}" that must be removed or replaced with the intended
rendered content; locate all literal "{response}" tokens in the document (there
are multiple occurrences) and either replace them with the correct review text
or remove them so the artifact contains only final, human-readable content.
In `@docs/FEATURES.md`:
- Around line 18-39: The features checklist in FEATURES.md skips DP-F-20 (it
jumps from DP-F-19 to DP-F-21); add a DP-F-20 entry in the list so the table of
contents matches the actual sections—insert an appropriately labeled line like
"- [ ] DP-F-20 <short title>" between the existing "- [ ] DP-F-19 Image Splash
(polish)" and "- [ ] DP-F-21 Doghouse Flight Recorder" entries and ensure the
label text matches the corresponding DP-F-20 section title elsewhere in the
document.
- Around line 245-345: The DP-US-0201 story is incorrectly nested under the
DP-F-21 feature causing taxonomy confusion; update the Markdown structure so
DP-US-0201 is placed under its correct feature header (e.g., create or reopen
the DP-F-02 top-level feature section) or close the DP-F-21 section before
introducing DP-US-0201, ensuring header levels match the pattern used for
DP-F-21/DP-US-2101/DP-US-2102 so automation can map DP-F-02 ↔ DP-US-0201
unambiguously.
In `@docs/SPRINTS.md`:
- Around line 5-8: The "Cadence & Dates" section contains a stale start date
("Start date: Monday, 2025-11-10") and ambiguous code-freeze timing ("Code
freeze on Fridays") — either update the start date(s) to current/future sprint
dates and adjust any recurring schedule to reflect today’s calendar, or
explicitly mark the section as historical/archived with a clear note (e.g.,
"Historical: plan used from Nov 2025 to ...") so readers aren’t confused; edit
the "Cadence & Dates" heading and the lines "Sprint length: 1 week (Mon–Fri)",
"Start date: Monday, 2025-11-10", and "Code freeze on Fridays; demo + retro on
Fridays 3pm local" accordingly.
- Around line 164-169: Add missing blank lines around the Markdown headings so
they are separated from surrounding paragraphs: insert a blank line before the
"## Backlog / Nice-to-Haves (Post-SPEC)" heading and likewise before the
"DP-F-19 Image Splash (bunbun.webp) behind `DP_TUI_IMAGE` (polish)" heading (and
ensure a blank line after the preceding paragraph "Merge/stash flows as
follow‑ups."). This fixes the Markdown formatting violation by ensuring headings
are preceded and followed by an empty line.
In `@doghouse/README.md`:
- Around line 37-39: Remove trailing whitespace from the two markdown lines that
end with extra spaces: the line starting "It reminded us of a small beagle named
**Snoopy**, sitting atop his wooden house, dreaming he was an ace pilot in ze
Great War, dogfighting ze Red Baron in ze clouds." and the line starting "When
you use zis tool, you are Snoopy. Your PR is your cockpit. You are sparring mit
ze reviewers—ze CodeRabbits und ze maintainers—in a tactical dance. Ze Doghouse
is your vessel, your Black Box, und your Sopwith Camel."; edit those lines to
delete the trailing spaces at the end of each line so markdownlint MD009
warnings are resolved.
In `@Makefile`:
- Around line 18-19: The Makefile's history target invokes a non-existent CLI
command "doghouse.cli.main history"; update the target to call one of the
registered commands (e.g., replace "doghouse.cli.main history" with
"PYTHONPATH=src $(PYTHON) -m doghouse.cli.main playback") or remove the history
target; reference the Makefile target name "history" and the CLI module
"doghouse.cli.main" and use an existing command like "playback" (available
commands: snapshot, playback, export, watch).
- Line 1: The .PHONY declaration is incomplete and there is no default
entrypoint; update the Makefile so the .PHONY line includes watch and export
(and any other phony targets like test, snapshot, history, playback) and add a
default "all" target (or equivalent entrypoint) that depends on the primary
build/dev target (e.g., dev-venv or the main composite target) so make has a
predictable default; reference the .PHONY declaration and add an "all:" target
that invokes the desired default dependency.
In `@PRODUCTION_LOG.mg`:
- Around line 27-30: The "Initial Entries" header text is now incorrect because
incidents are appended below; update the PRODUCTION_LOG.mg content by either
removing the "Initial Entries" section entirely or replacing its text with an
accurate statement (e.g., "Initial Entries: see incidents below" or a summary of
current entries), and ensure the header reflects the actual log state so it no
longer contradicts appended incidents.
- Around line 60-61: The file contains literal "\n" escape sequences committed
into the markdown (e.g., the string "\n## 2026-03-27: Doghouse Reboot (The Great
Pivot)\n- Deleted legacy Draft Punks..." and the following block) which break
rendering; open PRODUCTION_LOG.mg, find the escaped sequence (search for "\n##
2026-03-27" or the literal "\n" before the headings), remove the
backslash-escaped "\n" artifacts and replace them with actual newlines so the
headings ("## 2026-03-27: Doghouse Reboot..." and "## 2026-03-27: Soul
Restored") and list items become proper markdown lines, then save/commit the
cleaned markdown.
In `@pyproject.toml`:
- Around line 31-37: The file has inconsistent leading spaces before array items
and the script entry (under [project.optional-dependencies] dev and
[project.scripts] doghouse = "doghouse.cli.main:app"); normalize indentation by
removing the extra leading spaces so keys and values align with the rest of the
file (make the "dev" array items and the "doghouse" script entry have the same
left-alignment as surrounding entries).
- Line 8: The pyproject.toml readme entry references a missing file via the
readme field; fix by either creating the referenced file (create cli/README.md
with appropriate package README), or update the readme field to point to an
existing README (for example "README.md" or "doghouse/README.md"), or remove the
readme = {...} entry entirely so packaging won’t reference a non-existent file;
ensure the change is made to the readme field in pyproject.toml.
In `@README.md`:
- Around line 140-150: Markdown headings "📡 Capture a Sortie" and "🎬 Run a
Playback" and their fenced code blocks lack surrounding blank lines, causing
MD022/MD031 lint errors; add a blank line above each heading and ensure there is
an empty line before and after each triple-backtick fenced block (the blocks
containing `doghouse snapshot` and `doghouse playback pb1_push_delta`) so the
headings and code fences are separated from adjacent text and the document is
lint-clean.
In `@src/doghouse/adapters/git/git_adapter.py`:
- Line 13: The subprocess.run calls in git_adapter.py that capture git output
(for example the assignment to the variable status using subprocess.run(["git",
"status", "--porcelain"], ...) and the two other subprocess.run invocations
later in the same module) must explicitly declare check=False to document that
failures are intentionally ignored; update each subprocess.run call in this file
to include the keyword argument check=False while keeping existing
capture_output/text arguments unchanged.
- Around line 1-3: The file imports typing.List which is deprecated; replace the
typing.List import and all uses with the built-in list type—remove "from typing
import List" and update any type annotations that reference List (e.g., function
signatures or variables in git_adapter.py) to use "list" instead while keeping
other imports like Blocker, BlockerType, BlockerSeverity unchanged; ensure
annotations such as List[Something] become list[Something] (or just list if
unparametrized).
- Around line 27-30: The subprocess call that computes `unpushed` using ["git",
"rev-list", f"@{'{'}u{'}'}..HEAD"] can silently fail when the branch has no
upstream (exit code 128) because you only inspect stdout; replace the inline
escaped braces with a simple variable like upstream_ref = "@{u}" and call
subprocess.run(..., capture_output=True, text=True) into a variable (e.g.,
result), then check result.returncode and result.stderr: if returncode != 0
handle the error path (detect code 128 or inspect stderr) by logging/raising a
clear message that no upstream is configured or by fallback logic, otherwise use
result.stdout.strip() as before to compute `unpushed`; update any callers of
`unpushed` accordingly (reference the `unpushed` variable and the subprocess.run
invocation in git_adapter.py).
In `@src/doghouse/adapters/github/gh_cli_adapter.py`:
- Around line 50-77: The GraphQL query in gql_query uses reviewThreads(first:
100) which truncates results past the first page; update the query to request
pageInfo { hasNextPage endCursor } and accept an after: String variable (e.g.
reviewThreads(first: 100, after: $after)), then implement a loop in the method
that calls _run_gh_json to repeatedly fetch pages, passing the current endCursor
into the after variable and concatenating nodes into threads until
pageInfo.hasNextPage is false; ensure you accumulate comments from each page and
replace the single-page assignment to threads with the aggregated list before
further processing.
- Around line 15-22: The _run_gh helper uses subprocess.run(..., check=True)
with no timeout which can hang the whole recorder; update the _run_gh function
to pass a sensible timeout (e.g., 30s or configurable) to subprocess.run, catch
subprocess.TimeoutExpired and subprocess.CalledProcessError, and convert those
into controlled errors/ log messages (or raise a custom exception) so
snapshot/watch/export won't dead-stop on hung gh calls; ensure you still include
args and self.repo when building the cmd and document the timeout as
configurable if appropriate.
In `@src/doghouse/adapters/storage/jsonl_adapter.py`:
- Around line 26-29: save_snapshot currently appends directly which can leave a
truncated JSONL line that breaks readers; update save_snapshot (and the same
logic used at lines ~37-40) to perform an atomic append and make the readers
resilient: implement write-by-write atomicity by writing the new snapshot JSON
to a temporary file in the same directory, fsyncing the temp file, then
atomically replacing the target file (or swapping in the combined content) so a
partial write cannot be observed, and ensure you fsync the parent directory
after rename; additionally, update get_latest_snapshot and export to catch
json.JSONDecodeError when reading lines from the JSONL produced by _get_path,
skip/ignore any malformed/truncated lines at the file tail, and continue
processing valid snapshots so a single torn line no longer breaks the recorder.
In `@src/doghouse/cli/main.py`:
- Around line 46-49: The adapters are being instantiated without the selected
repo context so --repo is cosmetic and can cause cross-repo storage; update
GhCliAdapter, JSONLStorageAdapter (and DeltaEngine if it uses repo-scoped state)
to accept and store repo_owner/repo_name (or a single "repo" string) in their
constructors, then pass the CLI-selected repo into the instances created in
main.py (the github, storage, engine variables) and wherever else those adapters
are created (the other spots referenced around the file: the locations creating
the adapters at lines ~184-185 and ~222-225). Also ensure
RecorderService.record_sortie continues to receive repo and uses the adapter
instances tied to that repo rather than relying on the current checkout.
- Around line 129-131: The playback_path is currently resolved relative to the
current working directory (playback_path) which breaks when run as an installed
console script or from a subdirectory; change resolution to locate fixtures
relative to the package module instead (e.g., derive a base_dir from this
module's __file__ or use importlib.resources.files for the package) and then
build playback_path = base_dir / "fixtures" / "playbacks" / name, keeping the
same existence check and console.print error if missing; update any references
to playback_path accordingly.
- Around line 53-65: The current as_json branch uses
console.print(json.dumps(...)) which passes machine JSON through Rich
(console.print) causing markup interpretation; instead write the serialized JSON
string directly to stdout (e.g., use print(...) or sys.stdout.write(...) with
the json.dumps(...) result and a trailing newline) and remove console.print
usage; update the as_json branch that builds output from snapshot.to_dict() and
delta (baseline_timestamp, head_changed, added_blockers, removed_blockers,
verdict) to emit raw JSON so Rich markup won’t corrupt brackets or other
characters.
In `@src/doghouse/core/domain/blocker.py`:
- Around line 21-28: The snapshot serialization is dropping Blocker.is_primary
so secondary blockers are reloaded as primary; update the blocker serialization
and deserialization in src/doghouse/core/domain/snapshot.py (the
to_dict()/from_dict() or equivalent serialize_blocker/deserialize_blocker
functions) to include and read the is_primary field from the dict, preserving
the boolean into/out of the Blocker dataclass (referencing the Blocker class and
its is_primary attribute).
In `@src/doghouse/core/domain/delta.py`:
- Around line 31-50: The verdict logic currently treats all blockers equally;
change it to prefer primary blockers first by filtering self.added_blockers +
self.still_open_blockers into primary and secondary groups (use the
blocker.is_primary flag), then run the existing priority checks
(BlockerType.FAILING_CHECK, UNRESOLVED_THREAD, PENDING_CHECK) against the
primary list first and only if none match, repeat the same checks against the
secondary list; update the default message to count items from the appropriate
group (primary if any primary blockers exist, otherwise both) so the message
reflects primary-first ordering.
In `@src/doghouse/core/domain/snapshot.py`:
- Around line 24-27: The snapshot serialization is shallow: references to
self.metadata and each blocker .metadata are reused, allowing external mutation;
update the Snapshot serialization/deserialization logic (the to_dict/from_dict
or serialize/deserialize methods that build the dict with "metadata" and iterate
self.blockers) to return deep-copied metadata structures (e.g., use
copy.deepcopy on self.metadata and on each blocker.metadata when building the
dict and when reconstructing blockers) so the Snapshot and its Blocker objects
own immutable copies rather than shared dict references.
In `@src/doghouse/core/ports/github_port.py`:
- Around line 1-21: The interface GitHubPort uses deprecated typing aliases and
has redundant pass statements; update the imports to remove typing.Dict and
typing.List (use built-in dict and list types) and keep only needed typing names
(e.g., Optional, Any), then remove the unnecessary "pass" lines inside the
abstract methods get_head_sha, fetch_blockers, and get_pr_metadata so each
abstractmethod only contains its docstring and signature.
- Around line 8-11: The get_head_sha signature uses Optional[int] = None but
lacks a documented contract for None; update the get_head_sha method docstring
to explicitly state what implementations must do when pr_id is None (e.g., infer
the PR from local git context and return its head SHA, or raise a clear
ValueError/NotImplementedError if inference isn’t possible), and ensure any
concrete implementors of get_head_sha (and callers like RecorderService) follow
that contract (either handle None by inferring from git or validate and raise);
reference the get_head_sha abstract method and RecorderService call sites so
implementors/tests can be adjusted to cover the None-path or to remove Optional
if None should not be supported.
In `@src/doghouse/core/ports/storage_port.py`:
- Around line 1-21: The StoragePort interface currently imports typing.List and
includes redundant pass statements after the abstract method docstrings; update
the method signatures in StoragePort (save_snapshot, list_snapshots,
get_latest_snapshot) to use the built-in list type instead of typing.List
(remove the List import), and delete the unnecessary pass statements after each
docstring so the abstract methods contain only their docstrings and decorators
remain intact (keep ABC and `@abstractmethod` usage and Optional as-is).
In `@src/doghouse/core/services/delta_engine.py`:
- Around line 18-20: The Delta is being constructed with a direct reference to
current.blockers which lets downstream mutations change the Delta; when creating
the Delta (the call that sets added_blockers=current.blockers), pass a shallow
copy of the list instead (e.g., use list(current.blockers) or
current.blockers.copy()) so the Delta owns its own list instance and downstream
mutations to current.blockers won't affect the delta result.
- Around line 30-41: The Delta lists are built from set-derived ID collections
(baseline_ids, current_ids, still_open_ids) which yields nondeterministic order;
change the list comprehensions that build added_blockers, removed_blockers, and
still_open_blockers in the Delta return to iterate over a deterministic, sorted
sequence of IDs (e.g., sorted(added_ids), sorted(removed_ids),
sorted(still_open_ids) or sorted(..., key=...) if a specific ordering is
required) and map each sorted id through current_map/baseline_map so Delta (and
playback/JSON output) is stable across runs.
In `@src/doghouse/core/services/playback_service.py`:
- Around line 19-25: Remove the redundant "r" mode when calling open() for
current_path and baseline_path and add explicit FileNotFoundError handling
around reading current.json so you don't propagate an unhelpful traceback; wrap
the open/JSON load/Snapshot.from_dict sequence for current in a try/except that
catches FileNotFoundError and raises or logs a clearer error that includes
current_path and context (e.g., in the block using Snapshot.from_dict for
current) and optionally do the same for baseline_path when baseline is expected,
referencing current_path, baseline_path, and Snapshot.from_dict to locate the
code to change.
- Around line 1-6: The file imports deprecated typing constructs and omits the
__init__ return annotation; replace "from typing import Tuple, Optional" with no
typing imports and use native generics and union syntax (e.g., use
tuple[Snapshot, Delta] instead of Tuple[...] and Snapshot | None instead of
Optional[Snapshot]) throughout the module (check any function signatures that
reference Tuple or Optional), and add the missing return annotation "-> None" to
the class initializer method "__init__" (and update any other functions to use
built-in generics/unions), keeping references to Snapshot, Delta, and
DeltaEngine intact.
- Line 14: The declared return type for run_playback is incorrect because
baseline can be None when baseline_path doesn't exist; update the signature to
reflect this by changing the return type from Tuple[Snapshot, Snapshot, Delta]
to Tuple[Optional[Snapshot], Snapshot, Delta] (import Optional from typing) and
adjust any callers that assume baseline is always a Snapshot to handle None;
locate the run_playback function and the baseline/baseline_path handling to make
this change.
In `@src/doghouse/core/services/recorder_service.py`:
- Around line 31-36: The code concatenates remote_blockers and local_blockers
into blockers which can contain duplicate blocker entries and corrupt delta
semantics; update the logic in the recorder service (around remote_blockers,
local_blockers, and blockers) to deduplicate by blocker ID before snapshotting —
e.g., collect blockers into a map keyed by the unique ID (use blocker['id'] or
blocker.id consistent with your Blocker shape), merging or preferring
remote/local as desired, then build the final blockers list from the map and use
that for subsequent calls (e.g., where metadata is fetched and snapshotting
occurs).
In `@tests/doghouse/test_delta_engine.py`:
- Around line 6-28: Add tests to cover edge cases for DeltaEngine.compute_delta:
create new test functions (e.g., test_compute_delta_empty_blockers,
test_compute_delta_overlapping_blockers, test_compute_delta_mutated_blocker)
that exercise Snapshot with empty blockers for both baseline and current,
overlapping blocker lists where some persist while others are added/removed, and
cases where Blocker objects share the same id but differ in type or message to
ensure mutation detection; use the existing patterns in
test_compute_delta_no_changes to instantiate DeltaEngine, Snapshot, and Blocker,
call compute_delta, and assert baseline_sha/current_sha, head_changed, and the
lengths and contents of added_blockers, removed_blockers, and
still_open_blockers to validate expected behavior.
- Line 11: The test constructs naive datetimes (e.g., datetime.datetime(2026, 1,
1)) which will mismatch fixture timestamps parsed as UTC; update all four
datetime.datetime(...) constructions in tests/doghouse/test_delta_engine.py to
be timezone-aware by adding tzinfo=datetime.timezone.utc (e.g.,
datetime.datetime(2026, 1, 1, tzinfo=datetime.timezone.utc)) so comparisons with
Snapshot.from_dict-parsed fixtures (which use "Z") are consistent. Ensure you
update each of the four occurrences and keep using the same datetime module
symbol (datetime) used in the file.
In `@tools/bootstrap-git-mind.sh`:
- Around line 7-13: The script currently only prevents clobbering when $DEST
contains a .git directory; add a guard that rejects an existing non-empty
directory to avoid partial overwrites: after evaluating $DEST/.git, check $DEST
for an existing directory with contents (e.g., test -d "$DEST" and non-empty via
listing or POSIX-safe check) and print an error and exit if so; only proceed to
mkdir -p "$DEST" when $DEST does not exist or is an empty directory. Reference
the DEST variable and the existing mkdir -p "$DEST" and .git check to locate
where to insert this additional directory-non-empty validation.
- Around line 35-37: The cp command copying src/git_mind/* will cause the script
to exit under set -euo pipefail when the source directory or glob is missing;
update the bootstrap-git-mind.sh logic to first test the source (e.g., [ -d
src/git_mind ] and check it contains files) before running cp, and only
create/rsync/copy into "$DEST/src/git_mind/" when content exists; reference the
existing cp invocations (cp -R src/git_mind/* "$DEST/src/git_mind/" and the
other cp lines using "$DEST") and wrap them with an existence/non-empty check so
the script no longer hard-fails when src/git_mind is absent.
---
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 1-21: The workflow named "CI" (jobs.test) lacks an explicit
top-level permissions block; add a top-level permissions mapping to restrict the
GITHUB_TOKEN to least-privilege (for example set contents: read and
pull-requests: write or other minimal scopes your tests/actions need) so the
token scope is explicit and limited; place the permissions block directly under
the workflow header (near "name: CI") so jobs use the constrained token.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 03ec8f34-ce18-4148-8041-8d4a4b56190e
📒 Files selected for processing (65)
.github/workflows/ci.yml.github/workflows/publish.ymlMakefilePRODUCTION_LOG.mgREADME.mddocs/FEATURES.mddocs/SPRINTS.mddocs/TASKLIST.mddocs/archive/CLI-STATE.mddocs/archive/DRIFT_REPORT.mddocs/archive/IDEAS.mddocs/archive/INTEGRATIONS-git-kv.mddocs/archive/SPEC.mddocs/archive/STORY.mddocs/archive/TECH-SPEC.mddocs/archive/mind/DRIFT_REPORT.mddocs/archive/mind/FEATURES.mddocs/archive/mind/SPEC.mddocs/archive/mind/SPRINTS.mddocs/archive/mind/TASKLIST.mddocs/archive/mind/TECH-SPEC.mddocs/code-reviews/PR1/27b99435126e3d7a58706a4f6e0d20a5c02b1608.mddocs/code-reviews/PR1/85ac499f573fd79192a02aae02d2b0d97fcbc8c8.mddocs/code-reviews/PR2/016d60dfc0bc1175f093af3d78848df56c2dc787.mddocs/code-reviews/PR2/410ce3439856a4cdf6ae7df8fd3936e7bbb67bba.mddocs/code-reviews/PR2/6255c785ffa405438af63db62fe58541dfa200fb.mddocs/code-reviews/PR2/8ccf6beebb570b4ad0bf42e6d4489bbc1f2609e8.mddocs/code-reviews/PR2/d0185ed74890c49a762779a94fd4c22effd2a5ea.mddocs/code-reviews/PR5/56964e6b72bbe7639f9c725c6e9f2327f75bb402.mddocs/code-reviews/PR5/aee587e7aad9af37f73dd997dfbdef8dcbb53b04.mddoghouse/README.mddoghouse/flight-recorder-brief.mddoghouse/playbacks.mdexamples/8dfbfab49b290a969ed7bb6248f3880137ef177d.mdexamples/config.sample.jsonprompt.mdpyproject.tomlsrc/doghouse/__init__.pysrc/doghouse/adapters/__init__.pysrc/doghouse/adapters/git/__init__.pysrc/doghouse/adapters/git/git_adapter.pysrc/doghouse/adapters/github/__init__.pysrc/doghouse/adapters/github/gh_cli_adapter.pysrc/doghouse/adapters/storage/__init__.pysrc/doghouse/adapters/storage/jsonl_adapter.pysrc/doghouse/cli/__init__.pysrc/doghouse/cli/main.pysrc/doghouse/core/__init__.pysrc/doghouse/core/domain/__init__.pysrc/doghouse/core/domain/blocker.pysrc/doghouse/core/domain/delta.pysrc/doghouse/core/domain/snapshot.pysrc/doghouse/core/ports/__init__.pysrc/doghouse/core/ports/github_port.pysrc/doghouse/core/ports/storage_port.pysrc/doghouse/core/services/__init__.pysrc/doghouse/core/services/delta_engine.pysrc/doghouse/core/services/playback_service.pysrc/doghouse/core/services/recorder_service.pytests/doghouse/fixtures/playbacks/pb1_push_delta/baseline.jsontests/doghouse/fixtures/playbacks/pb1_push_delta/current.jsontests/doghouse/fixtures/playbacks/pb2_merge_ready/baseline.jsontests/doghouse/fixtures/playbacks/pb2_merge_ready/current.jsontests/doghouse/test_delta_engine.pytools/bootstrap-git-mind.sh
💤 Files with no reviewable changes (7)
- docs/code-reviews/PR2/016d60dfc0bc1175f093af3d78848df56c2dc787.md
- docs/code-reviews/PR1/85ac499f573fd79192a02aae02d2b0d97fcbc8c8.md
- docs/code-reviews/PR2/8ccf6beebb570b4ad0bf42e6d4489bbc1f2609e8.md
- docs/code-reviews/PR2/6255c785ffa405438af63db62fe58541dfa200fb.md
- docs/code-reviews/PR1/27b99435126e3d7a58706a4f6e0d20a5c02b1608.md
- docs/code-reviews/PR2/410ce3439856a4cdf6ae7df8fd3936e7bbb67bba.md
- docs/code-reviews/PR2/d0185ed74890c49a762779a94fd4c22effd2a5ea.md
Comment on lines
+59
to
+74
| 11) DP-F-10 Prompt Editing & Templates | ||
| - Missing: Editor flow; template tokens for context. | ||
|
|
||
| 12) DP-F-11 Settings & Persistence | ||
| - Missing: Dedicated Settings screen (reply_on_success, force_json, provider, etc.). | ||
|
|
||
| 13) DP-F-12 Merge Flow | ||
| - Missing completely. | ||
|
|
||
| 14) DP-F-13 Stash Dirty Changes Flow | ||
| - Missing completely (no dirty banner/flow). | ||
|
|
||
| 15) DP-F-15 Status Bar & Key Hints | ||
| - Missing persistent hints; Help overlay exists but not context bar. | ||
|
|
||
| 16) DP-F-16 Theming & Layout |
Contributor
There was a problem hiding this comment.
Ordered list numbering violates configured markdown style (MD029).
Lines [59]-[74] use explicit 11) … 16) prefixes in a list style configured as 1/1/1. Normalize the list numbering to satisfy markdownlint.
Suggested fix pattern
-11) DP-F-10 Prompt Editing & Templates
-12) DP-F-11 Settings & Persistence
-13) DP-F-12 Merge Flow
-14) DP-F-13 Stash Dirty Changes Flow
-15) DP-F-15 Status Bar & Key Hints
-16) DP-F-16 Theming & Layout
+1. DP-F-10 Prompt Editing & Templates
+1. DP-F-11 Settings & Persistence
+1. DP-F-12 Merge Flow
+1. DP-F-13 Stash Dirty Changes Flow
+1. DP-F-15 Status Bar & Key Hints
+1. DP-F-16 Theming & Layout📝 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
| 11) DP-F-10 Prompt Editing & Templates | |
| - Missing: Editor flow; template tokens for context. | |
| 12) DP-F-11 Settings & Persistence | |
| - Missing: Dedicated Settings screen (reply_on_success, force_json, provider, etc.). | |
| 13) DP-F-12 Merge Flow | |
| - Missing completely. | |
| 14) DP-F-13 Stash Dirty Changes Flow | |
| - Missing completely (no dirty banner/flow). | |
| 15) DP-F-15 Status Bar & Key Hints | |
| - Missing persistent hints; Help overlay exists but not context bar. | |
| 16) DP-F-16 Theming & Layout | |
| 1. DP-F-10 Prompt Editing & Templates | |
| - Missing: Editor flow; template tokens for context. | |
| 1. DP-F-11 Settings & Persistence | |
| - Missing: Dedicated Settings screen (reply_on_success, force_json, provider, etc.). | |
| 1. DP-F-12 Merge Flow | |
| - Missing completely. | |
| 1. DP-F-13 Stash Dirty Changes Flow | |
| - Missing completely (no dirty banner/flow). | |
| 1. DP-F-15 Status Bar & Key Hints | |
| - Missing persistent hints; Help overlay exists but not context bar. | |
| 1. DP-F-16 Theming & Layout |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 59-59: Ordered list item prefix
Expected: 1; Actual: 11; Style: 1/1/1
(MD029, ol-prefix)
[warning] 62-62: Ordered list item prefix
Expected: 1; Actual: 12; Style: 1/1/1
(MD029, ol-prefix)
[warning] 65-65: Ordered list item prefix
Expected: 1; Actual: 13; Style: 1/1/1
(MD029, ol-prefix)
[warning] 68-68: Ordered list item prefix
Expected: 1; Actual: 14; Style: 1/1/1
(MD029, ol-prefix)
[warning] 71-71: Ordered list item prefix
Expected: 1; Actual: 15; Style: 1/1/1
(MD029, ol-prefix)
[warning] 74-74: Ordered list item prefix
Expected: 1; Actual: 16; Style: 1/1/1
(MD029, ol-prefix)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/archive/DRIFT_REPORT.md` around lines 59 - 74, The ordered list in the
DRIFT_REPORT.md section containing items "DP-F-10 Prompt Editing & Templates"
through "DP-F-16 Theming & Layout" uses explicit numbered prefixes like
"11)"–"16)" which violates MD029; update that block so each list item uses the
configured normalized numbering (e.g. start each line with "1." or remove
explicit numbers to allow automatic numbering) for the entries referencing
DP-F-10, DP-F-11, DP-F-12, DP-F-13, DP-F-15, and DP-F-16 so markdownlint accepts
the 1/1/1 style.
Comment on lines
+245
to
+345
| ## DP-F-21 Doghouse Flight Recorder | ||
|
|
||
| ### DP-US-2101 Capture Sortie State | ||
|
|
||
| #### User Story | ||
|
|
||
| | | | | ||
| |--|--| | ||
| | **As a** | PR author | | ||
| | **I want** | a durable local snapshot of the current review sortie | | ||
| | **So that** | I can recover the live PR state without rereading GitHub from scratch. | | ||
|
|
||
| - [ ] Done | ||
|
|
||
| #### Requirements | ||
|
|
||
| - [ ] Capture head SHA, unresolved thread set, grouped checks, review decision, merge state, and reviewer-specific gating such as CodeRabbit pause/cooldown state. | ||
| - [ ] Write local machine-readable artifacts that can be compared later. | ||
| - [ ] Treat human reviewer state separately from automated reviewer state. | ||
|
|
||
| #### Acceptance Criteria | ||
|
|
||
| - [ ] After a push, the operator can capture a fresh sortie and recover the exact current blocker set locally. | ||
| - [ ] The artifact can be loaded later without access to terminal scrollback. | ||
|
|
||
| #### Test Plan | ||
|
|
||
| - [ ] Fixture-based snapshot serialization tests. | ||
| - [ ] Adapter tests for PR state collection. | ||
|
|
||
| ### DP-US-2102 Compute Semantic Delta | ||
|
|
||
| #### User Story | ||
|
|
||
| | | | | ||
| |--|--| | ||
| | **As a** | PR author | | ||
| | **I want** | a semantic delta against the last meaningful sortie | | ||
| | **So that** | I can tell what changed and what I should do next. | | ||
|
|
||
| - [ ] Done | ||
|
|
||
| #### Requirements | ||
|
|
||
| - [ ] Prefer meaningful baselines over raw "last file written" comparisons. | ||
| - [ ] Track blocker, thread, and check transitions. | ||
| - [ ] Emit a machine-usable next-action verdict. | ||
|
|
||
| #### Acceptance Criteria | ||
|
|
||
| - [ ] The tool can distinguish "wait for pending checks" from "fix unresolved threads" from "merge-ready pending approval." | ||
| - [ ] The delta ignores raw timestamp churn and reordered arrays. | ||
|
|
||
| #### Test Plan | ||
|
|
||
| - [ ] State-transition fixtures. | ||
| - [ ] Replay tests for representative PR scenarios. | ||
|
|
||
| ### DP-US-0201 Fetch and Render PR List | ||
|
|
||
| #### User Story | ||
|
|
||
| | | | | ||
| |--|--| | ||
| | **As a** | User | | ||
| | **I want** | to fetch and Render PR List | | ||
| | **So that** | so I can choose the right PR quickly. | | ||
|
|
||
|
|
||
| - [ ] Done | ||
|
|
||
| #### Requirements | ||
|
|
||
| - [ ] Use GitHub Port to fetch open PRs; | ||
| - [ ] render per SPEC: | ||
| - [ ] icon (✅🟡🛑🚫), | ||
| - [ ] number, | ||
| - [ ] `{ i, e }`, | ||
| - [ ] branch, | ||
| - [ ] author, | ||
| - [ ] age, | ||
| - [ ] truncated title (≤50 chars with `[…]`). | ||
|
|
||
| #### Acceptance Criteria | ||
|
|
||
| - [ ] Visuals match SPEC examples; | ||
| - [ ] Enter on a PR navigates to PR View. | ||
|
|
||
| #### DoR | ||
|
|
||
| - [ ] Adapter returns head branch, | ||
| - [ ] author login, | ||
| - [ ] CI state, | ||
| - [ ] issue/error counts or `None`. | ||
|
|
||
| #### Test Plan | ||
|
|
||
| - [ ] Fake adapter; | ||
| - [ ] snapshot of three PRs; | ||
| - [ ] age humanizer unit tests. | ||
|
|
Contributor
There was a problem hiding this comment.
Feature/story ID taxonomy is broken by section nesting.
Line [303] starts DP-US-0201 (DP-F-02 namespace) while it is still nested under ## DP-F-21 from Line [245]. This breaks ID-to-feature mapping and makes the catalog ambiguous for automation/reporting.
Suggested structural correction
## DP-F-02 Main Menu — PR Selection
----
-
## DP-F-21 Doghouse Flight Recorder
@@
### DP-US-2102 Compute Semantic Delta
@@
- [ ] Replay tests for representative PR scenarios.
+
+---
+
+## DP-F-02 Main Menu — PR Selection
+
+### DP-US-0201 Fetch and Render PR List🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 318-318: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 319-319: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 320-320: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 321-321: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 322-322: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 323-323: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 324-324: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 325-325: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 330-330: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 335-335: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 336-336: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 337-337: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 342-342: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 343-343: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/FEATURES.md` around lines 245 - 345, The DP-US-0201 story is incorrectly
nested under the DP-F-21 feature causing taxonomy confusion; update the Markdown
structure so DP-US-0201 is placed under its correct feature header (e.g., create
or reopen the DP-F-02 top-level feature section) or close the DP-F-21 section
before introducing DP-US-0201, ensuring header levels match the pattern used for
DP-F-21/DP-US-2101/DP-US-2102 so automation can map DP-F-02 ↔ DP-US-0201
unambiguously.
Makefile
Outdated
| @@ -0,0 +1,35 @@ | |||
| .PHONY: dev-venv test snapshot history playback clean | |||
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Harden phony declarations and add a default entrypoint.
Line [1] omits watch/export from .PHONY, and checkmake also flags missing all. Add both for predictable make behavior.
Suggested fix
-.PHONY: dev-venv test snapshot history playback clean
+.PHONY: all dev-venv test snapshot playback watch export clean
+
+all: test🧰 Tools
🪛 checkmake (0.2.2)
[warning] 1-1: Missing required phony target "all"
(minphony)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` at line 1, The .PHONY declaration is incomplete and there is no
default entrypoint; update the Makefile so the .PHONY line includes watch and
export (and any other phony targets like test, snapshot, history, playback) and
add a default "all" target (or equivalent entrypoint) that depends on the
primary build/dev target (e.g., dev-venv or the main composite target) so make
has a predictable default; reference the .PHONY declaration and add an "all:"
target that invokes the desired default dependency.
Comment on lines
+15
to
+22
| def _run_gh(self, args: List[str]) -> str: | ||
| """Execute a 'gh' command and return stdout.""" | ||
| cmd = ["gh"] + args | ||
| if self.repo: | ||
| cmd += ["-R", self.repo] | ||
|
|
||
| result = subprocess.run(cmd, capture_output=True, text=True, check=True) | ||
| return result.stdout |
Contributor
There was a problem hiding this comment.
Unbounded gh calls can dead-stop the whole recorder.
Every GitHub read funnels through this helper, but subprocess.run(..., check=True) has no timeout. One hung auth prompt or network stall freezes snapshot, watch, and export until the user kills the process.
Minimal hardening
- result = subprocess.run(cmd, capture_output=True, text=True, check=True)
+ result = subprocess.run(
+ cmd,
+ capture_output=True,
+ text=True,
+ check=True,
+ timeout=30,
+ )📝 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
| def _run_gh(self, args: List[str]) -> str: | |
| """Execute a 'gh' command and return stdout.""" | |
| cmd = ["gh"] + args | |
| if self.repo: | |
| cmd += ["-R", self.repo] | |
| result = subprocess.run(cmd, capture_output=True, text=True, check=True) | |
| return result.stdout | |
| def _run_gh(self, args: List[str]) -> str: | |
| """Execute a 'gh' command and return stdout.""" | |
| cmd = ["gh"] + args | |
| if self.repo: | |
| cmd += ["-R", self.repo] | |
| result = subprocess.run( | |
| cmd, | |
| capture_output=True, | |
| text=True, | |
| check=True, | |
| timeout=30, | |
| ) | |
| return result.stdout |
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 17-17: Consider ["gh", *args] instead of concatenation
Replace with ["gh", *args]
(RUF005)
[error] 21-21: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/doghouse/adapters/github/gh_cli_adapter.py` around lines 15 - 22, The
_run_gh helper uses subprocess.run(..., check=True) with no timeout which can
hang the whole recorder; update the _run_gh function to pass a sensible timeout
(e.g., 30s or configurable) to subprocess.run, catch subprocess.TimeoutExpired
and subprocess.CalledProcessError, and convert those into controlled errors/ log
messages (or raise a custom exception) so snapshot/watch/export won't dead-stop
on hung gh calls; ensure you still include args and self.repo when building the
cmd and document the timeout as configurable if appropriate.
Comment on lines
+50
to
+77
| gql_query = """ | ||
| query($owner: String!, $repo: String!, $pr: Int!) { | ||
| repository(owner: $owner, name: $repo) { | ||
| pullRequest(number: $pr) { | ||
| reviewThreads(first: 100) { | ||
| nodes { | ||
| isResolved | ||
| comments(first: 1) { | ||
| nodes { | ||
| body | ||
| id | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| """ | ||
| try: | ||
| gql_res = self._run_gh_json([ | ||
| "api", "graphql", | ||
| "-F", f"owner={owner}", | ||
| "-F", f"repo={name}", | ||
| "-F", f"pr={actual_pr_id}", | ||
| "-f", f"query={gql_query}" | ||
| ]) | ||
| threads = gql_res.get("data", {}).get("repository", {}).get("pullRequest", {}).get("reviewThreads", {}).get("nodes", []) |
Contributor
There was a problem hiding this comment.
The thread scan hard-stops at 100.
reviewThreads(first: 100) without pageInfo/endCursor silently drops everything after page one. On large PRs that undercounts unresolved-thread blockers and can hand the delta engine a false “merge ready” state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/doghouse/adapters/github/gh_cli_adapter.py` around lines 50 - 77, The
GraphQL query in gql_query uses reviewThreads(first: 100) which truncates
results past the first page; update the query to request pageInfo { hasNextPage
endCursor } and accept an after: String variable (e.g. reviewThreads(first: 100,
after: $after)), then implement a loop in the method that calls _run_gh_json to
repeatedly fetch pages, passing the current endCursor into the after variable
and concatenating nodes into threads until pageInfo.hasNextPage is false; ensure
you accumulate comments from each page and replace the single-page assignment to
threads with the aggregated list before further processing.
Comment on lines
+26
to
+29
| def save_snapshot(self, repo: str, pr_id: int, snapshot: Snapshot) -> None: | ||
| path = self._get_path(repo, pr_id) | ||
| with open(path, "a") as f: | ||
| f.write(json.dumps(snapshot.to_dict()) + "\n") |
Contributor
There was a problem hiding this comment.
One torn JSONL append poisons the entire recorder.
These writes go straight into the live log, and the read path trusts every line. If the process dies mid-append, the next json.loads() blows up get_latest_snapshot() and export() for that PR instead of recovering gracefully from a truncated tail record.
Also applies to: 37-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/doghouse/adapters/storage/jsonl_adapter.py` around lines 26 - 29,
save_snapshot currently appends directly which can leave a truncated JSONL line
that breaks readers; update save_snapshot (and the same logic used at lines
~37-40) to perform an atomic append and make the readers resilient: implement
write-by-write atomicity by writing the new snapshot JSON to a temporary file in
the same directory, fsyncing the temp file, then atomically replacing the target
file (or swapping in the combined content) so a partial write cannot be
observed, and ensure you fsync the parent directory after rename; additionally,
update get_latest_snapshot and export to catch json.JSONDecodeError when reading
lines from the JSONL produced by _get_path, skip/ignore any malformed/truncated
lines at the file tail, and continue processing valid snapshots so a single torn
line no longer breaks the recorder.
src/doghouse/cli/main.py
Outdated
Comment on lines
+46
to
+49
| github = GhCliAdapter() | ||
| storage = JSONLStorageAdapter() | ||
| engine = DeltaEngine() | ||
| service = RecorderService(github, storage, engine) |
Contributor
There was a problem hiding this comment.
--repo is cosmetic right now; this can corrupt history.
RecorderService.record_sortie(repo, pr) only uses repo for storage. Because these adapters are created without repo_owner/repo_name, the actual gh reads still target the current checkout, so snapshot/export/watch --repo other/repo can persist repo A’s state under repo B’s key.
Thread the selected repo into the adapter
+def _make_github_adapter(repo: str) -> GhCliAdapter:
+ owner, name = repo.split("/", 1)
+ return GhCliAdapter(repo_owner=owner, repo_name=name)
+
...
- github = GhCliAdapter()
+ github = _make_github_adapter(repo)
...
- github = GhCliAdapter()
+ github = _make_github_adapter(repo)
...
- github = GhCliAdapter()
+ github = _make_github_adapter(repo)Also applies to: 184-185, 222-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/doghouse/cli/main.py` around lines 46 - 49, The adapters are being
instantiated without the selected repo context so --repo is cosmetic and can
cause cross-repo storage; update GhCliAdapter, JSONLStorageAdapter (and
DeltaEngine if it uses repo-scoped state) to accept and store
repo_owner/repo_name (or a single "repo" string) in their constructors, then
pass the CLI-selected repo into the instances created in main.py (the github,
storage, engine variables) and wherever else those adapters are created (the
other spots referenced around the file: the locations creating the adapters at
lines ~184-185 and ~222-225). Also ensure RecorderService.record_sortie
continues to receive repo and uses the adapter instances tied to that repo
rather than relying on the current checkout.
…Harden CI/CD permissions and branch scope\n- Split build/publish steps in workflow\n- Fix GitAdapter import and missing __init__.py\n- Ensure Snapshot/Blocker immutability\n- Implement sorted IDs for deterministic delta\n- Fix GhCliAdapter GraphQL call and indentation\n- Fix JSON output encoding and --repo support\n- Harden subprocess calls with timeouts\n- Add expanded unit tests for edge cases\n- Update CHANGELOG and bump version to 0.1.0
Owner
Author
|
@coderabbitai I have addressed the feedback in recent commits:\n- Fixed CI/CD permissions and expanded branch scope\n- Split build/publish steps\n- Fixed GitAdapter imports and missing init.py\n- Ensured Snapshot/Blocker immutability and persistence of is_primary\n- Implemented sorted IDs for deterministic delta\n- Fixed GhCliAdapter GraphQL call and indentation\n- Fixed JSON output encoding and --repo support\n- Hardened subprocess calls with timeouts\n- Added expanded unit tests\n- Updated CHANGELOG and bumped version to 0.1.0\n- Removed trailing whitespace and updated docs\n\nReview please 🥕 |
Contributor
|
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (13)
docs/archive/SPRINTS.md (2)
5-8:⚠️ Potential issue | 🟡 MinorCadence section is date-stale and ambiguous.
Line 7 says “Monday, 2025-11-10,” which is already in the past (current date: March 28, 2026). Either update to active sprint dates or explicitly mark this section as historical to prevent planning confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/archive/SPRINTS.md` around lines 5 - 8, The "Cadence & Dates" section contains a stale start date ("Start date: Monday, 2025-11-10") that causes confusion; update the "Start date" line under the Cadence & Dates header to either a current/upcoming sprint start (e.g., compute and insert the next Monday date) or prepend an explicit marker like "Historical:" / "Archive:" to the entire Cadence & Dates block (including the "Sprint length" and "Code freeze" lines) so readers know these are past sprint details rather than active planning info.
168-177:⚠️ Potential issue | 🟡 MinorHeading spacing still violates MD022 (blank line required below headings).
Line 168 and Line 176 headings are immediately followed by list content. Insert one blank line after each heading.
Minimal markdown fix
## Backlog / Nice-to-Haves (Post-SPEC) + - DP-F-19 Image Splash (bunbun.webp) behind `DP_TUI_IMAGE` (polish). @@ ## Cross-Cutting Tech Debt & Risks + - Textual API drift (OptionList, ListView): maintain compatibility shims; pin minimum version.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/archive/SPRINTS.md` around lines 168 - 177, The Markdown headings "Backlog / Nice-to-Haves (Post-SPEC)" and "Cross-Cutting Tech Debt & Risks" violate MD022 by not having a blank line after the heading; fix by inserting a single blank line immediately after each of those headings so the lists that follow are separated (preserve existing list formatting and spacing otherwise) and rerun lint to confirm MD022 is resolved.docs/archive/SPEC.md (1)
5-1166:⚠️ Potential issue | 🟡 MinorMarkdown fence/lint debt is still pervasive and needs one cleanup pass.
Line 5 onward still contains many unlabeled fenced blocks (MD040), with previously reported whitespace/newline hygiene issues likely still mixed in. This keeps docs CI noisy and buries real regressions.
Suggested normalization pass
-``` +```text Title Screen └── Main Menu (PR Selection) ... -``` +``` -``` +```text # {title} ... -``` +``` -```mermaid +```mermaid graph TD ...Apply the same pattern repo-wide in this file:
textfor ASCII/mock UI blocksmermaidfor diagramstomlfor config examples- trim trailing spaces
- ensure a single trailing newline at EOF
</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/archive/SPEC.mdaround lines 5 - 1166, The file contains many unlabeled
fenced code blocks (MD040) and whitespace/newline hygiene issues starting at the
"Title Screen" / "0. Scroll View Widget" sections; fix by labeling all fenced
blocks according to their content (usetext for ASCII/UI art like the Title Screen and Scroll View examples,mermaid for diagrams, ```toml for the Config
Structure block, etc.), normalize the fence openings/closings so each block has
matching backticks, remove trailing spaces inside the file, and ensure exactly
one trailing newline at EOF; search for the unlabeled ASCII blocks, mermaid
diagrams, and the Config Structure sample to locate and update them.</details> </blockquote></details> <details> <summary>docs/FEATURES.md (1)</summary><blockquote> `242-346`: _⚠️ Potential issue_ | _🟠 Major_ **Feature/story namespace is still broken by incorrect section nesting.** Line 304 (`DP-US-0201`) is a `DP-F-02` story but currently sits under `## DP-F-21` (Line 246). That makes ID-based mapping ambiguous for tooling and reporting. <details> <summary>Suggested structural correction</summary> ```diff ## DP-F-02 Main Menu — PR Selection ---- - ## DP-F-21 Doghouse Flight Recorder @@ ### DP-US-2102 Compute Semantic Delta @@ - [ ] Replay tests for representative PR scenarios. + +--- + +## DP-F-02 Main Menu — PR Selection + +### DP-US-0201 Fetch and Render PR List ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@docs/FEATURES.md` around lines 242 - 346, The DP-US-0201 story block is nested under the wrong feature heading (it's currently placed beneath the DP-F-21 Doghouse Flight Recorder section); move the entire DP-US-0201 "Fetch and Render PR List" subsection (including its User Story, Requirements, Acceptance Criteria, DoR, and Test Plan) out from under the DP-F-21 section and insert it directly under the DP-F-02 "Main Menu — PR Selection" heading so that DP-US-0201 is a child of DP-F-02 and not DP-F-21, preserving the exact subsection content and markdown heading levels. ``` </details> </blockquote></details> <details> <summary>src/doghouse/core/services/delta_engine.py (1)</summary><blockquote> `13-20`: _⚠️ Potential issue_ | _🟡 Minor_ **No-baseline path still aliases `current.blockers` into `Delta`.** This leaks the same list instance into `Delta.added_blockers`; mutate one, mutate both. <details> <summary>✅ One-line fix</summary> ```diff - added_blockers=current.blockers, + added_blockers=list(current.blockers), ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/doghouse/core/services/delta_engine.py` around lines 13 - 20, The no-baseline branch of Delta construction currently assigns current.blockers directly to Delta.added_blockers, which aliases the same list; change the assignment in the Delta return in delta_engine.py to pass a shallow copy of current.blockers (e.g., create a new list from current.blockers) so mutations on Delta.added_blockers won’t affect current.blockers; keep other fields (removed_blockers, still_open_blockers) as-is. ``` </details> </blockquote></details> <details> <summary>src/doghouse/core/domain/snapshot.py (1)</summary><blockquote> `13-53`: _⚠️ Potential issue_ | _🟠 Major_ **Snapshot immutability is still shallow; metadata references can be mutated externally.** `dict(...)`/direct assignment here only copies top-level containers. Nested metadata in both snapshot-level and blocker-level payloads can still be mutated outside the model. <details> <summary>🛡️ Suggested hardening</summary> ```diff import datetime +import copy from dataclasses import dataclass, field, asdict from typing import List, Dict, Any, Optional @@ def __post_init__(self): # Ensure immutability by copying input lists/dicts - object.__setattr__(self, 'blockers', list(self.blockers)) - object.__setattr__(self, 'metadata', dict(self.metadata)) + object.__setattr__(self, "blockers", list(self.blockers)) + object.__setattr__(self, "metadata", copy.deepcopy(self.metadata)) @@ "is_primary": b.is_primary, "message": b.message, - "metadata": b.metadata + "metadata": copy.deepcopy(b.metadata), } for b in self.blockers ], - "metadata": self.metadata + "metadata": copy.deepcopy(self.metadata), } @@ severity=BlockerSeverity(b["severity"]), is_primary=b.get("is_primary", True), message=b["message"], - metadata=b.get("metadata", {}) + metadata=copy.deepcopy(b.get("metadata", {})), ) for b in data["blockers"] ], - metadata=data.get("metadata", {}) + metadata=copy.deepcopy(data.get("metadata", {})), ) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/doghouse/core/domain/snapshot.py` around lines 13 - 53, The Snapshot still allows external mutation of nested structures because __post_init__ and to_dict/from_dict use shallow copies or direct references; fix by using deep copies for snapshot-level metadata and blocker.metadata in __post_init__ (and import copy), ensure from_dict constructs Blocker.metadata and Snapshot.metadata via copy.deepcopy of input dicts, and have to_dict return deep-copied metadata/blocker.metadata so callers cannot mutate internal state; reference __post_init__, to_dict, from_dict, Snapshot, Blocker, and metadata/blockers when making the changes. ``` </details> </blockquote></details> <details> <summary>src/doghouse/adapters/storage/jsonl_adapter.py (1)</summary><blockquote> `26-41`: _⚠️ Potential issue_ | _🟠 Major_ **One malformed/torn JSONL line can brick history reads for that PR.** Direct append plus strict `json.loads` on every line means a partial tail write can break `list_snapshots()`/`get_latest_snapshot()` entirely. <details> <summary>🧯 Suggested resilience patch</summary> ```diff def save_snapshot(self, repo: str, pr_id: int, snapshot: Snapshot) -> None: path = self._get_path(repo, pr_id) - with open(path, "a") as f: - f.write(json.dumps(snapshot.to_dict()) + "\n") + line = json.dumps(snapshot.to_dict()) + "\n" + with open(path, "a", encoding="utf-8") as f: + f.write(line) + f.flush() + os.fsync(f.fileno()) @@ def list_snapshots(self, repo: str, pr_id: int) -> List[Snapshot]: @@ - with open(path, "r") as f: + with open(path, "r", encoding="utf-8") as f: for line in f: if line.strip(): - snapshots.append(Snapshot.from_dict(json.loads(line))) + try: + snapshots.append(Snapshot.from_dict(json.loads(line))) + except json.JSONDecodeError: + # tolerate torn/corrupt tail records + continue return snapshots ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/doghouse/adapters/storage/jsonl_adapter.py` around lines 26 - 41, The JSONL reader can be broken by a partial/torn append; update save_snapshot and list_snapshots to be resilient: in save_snapshot (function save_snapshot and helper _get_path) ensure each appended line is fully flushed to disk (call f.flush() then os.fsync(f.fileno())) after writing json.dumps(snapshot.to_dict()) + "\n" to reduce partial writes; in list_snapshots (and any get_latest_snapshot that reads the same file) make the per-line parsing robust by wrapping json.loads(...) and Snapshot.from_dict(...) in a try/except catching JSONDecodeError/ValueError and other mapping errors, skip malformed/empty lines, and optionally log a warning with the offending line so a single bad line no longer bricks reads. ``` </details> </blockquote></details> <details> <summary>src/doghouse/adapters/github/gh_cli_adapter.py (1)</summary><blockquote> `51-79`: _⚠️ Potential issue_ | _🟠 Major_ **Page one is not the whole PR.** `reviewThreads(first: 100)` truncates the connection. Large PRs lose unresolved threads after page one, so `fetch_blockers()` can hand the rest of the system a false “merge ready” snapshot. Paginate on `pageInfo { hasNextPage endCursor }` and loop with an `after` cursor until exhausted. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/doghouse/adapters/github/gh_cli_adapter.py` around lines 51 - 79, The current GraphQL query in gql_query and the logic that assigns threads from gql_res only fetches the first page (reviewThreads(first: 100)) and loses threads on large PRs; update fetch_blockers() to paginate: extend gql_query to request pageInfo { hasNextPage endCursor } and accept an optional $after: String variable, then loop calling self._run_gh_json (passing "-F", f"after={after}" each iteration) accumulating nodes into the threads list until pageInfo.hasNextPage is false, updating after = endCursor each loop so all reviewThreads are retrieved before downstream processing. ``` </details> </blockquote></details> <details> <summary>PRODUCTION_LOG.mg (1)</summary><blockquote> `27-29`: _⚠️ Potential issue_ | _🟡 Minor_ **The log is not empty anymore.** “Initial Entries” followed by “- (none yet)” is now false a few lines later. Remove the empty-state banner or replace it with a real summary. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@PRODUCTION_LOG.mg` around lines 27 - 29, The "Initial Entries" empty-state banner and the "- (none yet)" line are now inaccurate; remove the empty-state banner or replace it with an actual summary of current log contents by editing the "Initial Entries" header block and deleting the "- (none yet)" placeholder, or substitute it with a brief, factual summary of the entries present. ``` </details> </blockquote></details> <details> <summary>.github/workflows/ci.yml (1)</summary><blockquote> `9-11`: _⚠️ Potential issue_ | _🟠 Major_ **Revoke unnecessary `pull-requests: write` from CI workflow.** The test job executes only `pip install -e .[dev]` and `pytest` without invoking any GitHub API that requires PR write access. Granting `pull-requests: write` at workflow scope expands the token blast radius unnecessarily. Restrict permissions to `contents: read` unless a dedicated job actually comments on PRs. <details> <summary>Corrected permissions</summary> ```diff permissions: contents: read - pull-requests: write ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 9 - 11, Remove the unnecessary workflow permission "pull-requests: write" from the permissions block so the CI token is not granted PR write access; keep only "contents: read" (or the minimal permissions required) in the permissions: section and ensure no job in this workflow needs higher privileges before restoring any broader scope. ``` </details> </blockquote></details> <details> <summary>README.md (1)</summary><blockquote> `140-149`: _⚠️ Potential issue_ | _🟡 Minor_ **These headings/fences are still lint-dirty.** `markdownlint` will keep throwing `MD022`/`MD031` until the headings and fenced blocks are surrounded by blank lines here. Fix the structure; leave the voice alone. <details> <summary>Patch</summary> ```diff ### 📡 Capture a Sortie + Run zis to see what has changed since your last rehearsal. + ```bash doghouse snapshot ``` ### 🎬 Run a Playback + Verify the delta engine logic against offline scores (fixtures). + ```bash doghouse playback pb1_push_delta ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@README.mdaround lines 140 - 149, Add blank lines around the headings and
fenced code blocks to satisfy markdownlint MD022/MD031: ensure there is a blank
line before and after the "### 📡 Capture a Sortie" and "### 🎬 Run a Playback"
headings and also a blank line before the openingbash and after the closingfor bothdoghouse snapshotanddoghouse playback pb1_push_deltacode
fences so the README's structure is lint-clean while keeping the existing text
unchanged.</details> </blockquote></details> <details> <summary>src/doghouse/cli/main.py (2)</summary><blockquote> `34-39`: _⚠️ Potential issue_ | _🟠 Major_ **`history` is still not a command.** This Typer app registers `snapshot`, `playback`, `export`, and `watch` — nothing named `history`. So `doghouse history` and `make history` still fail at parse time. Either implement the command here or stop advertising it everywhere else. Also applies to: 131-135, 182-186, 219-225 --- `45-50`: _⚠️ Potential issue_ | _🔴 Critical_ **`--repo` still mixes foreign state into the wrong PR history.** `get_current_repo_and_pr()` always inspects the current checkout. If the user overrides only `--repo`, you pair repo B with repo A’s detected PR number. Then `export`/`watch` make it worse by constructing `GhCliAdapter()` without the selected repo, so they can read repo A while persisting/exporting under repo B’s key. That is silent cross-repo corruption. <details> <summary>Hard-stop the context mixup and bind the adapter once</summary> ```diff +def _resolve_repo_and_pr(repo: Optional[str], pr: Optional[int]) -> tuple[str, int]: + if repo is None and pr is None: + return get_current_repo_and_pr() + if repo is None: + detected_repo, _ = get_current_repo_and_pr() + return detected_repo, pr + if pr is None: + raise typer.BadParameter("--pr is required when --repo is set") + return repo, pr + + +def _make_github_adapter(repo: str) -> GhCliAdapter: + owner, name = repo.split("/", 1) + return GhCliAdapter(repo_owner=owner, repo_name=name) + + def snapshot(...): - if not repo or not pr: - detected_repo, detected_pr = get_current_repo_and_pr() - repo = repo or detected_repo - pr = pr or detected_pr + repo, pr = _resolve_repo_and_pr(repo, pr) ... - github = GhCliAdapter(repo_owner=repo_owner, repo_name=repo_name) + github = _make_github_adapter(repo) ... def export(...): - if not repo or not pr: - detected_repo, detected_pr = get_current_repo_and_pr() - repo = repo or detected_repo - pr = pr or detected_pr + repo, pr = _resolve_repo_and_pr(repo, pr) ... - github = GhCliAdapter() + github = _make_github_adapter(repo) ... def watch(...): - if not repo or not pr: - detected_repo, detected_pr = get_current_repo_and_pr() - repo = repo or detected_repo - pr = pr or detected_pr + repo, pr = _resolve_repo_and_pr(repo, pr) ... - github = GhCliAdapter() + github = _make_github_adapter(repo) ``` </details> Also applies to: 188-197, 226-237 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/doghouse/cli/main.py` around lines 45 - 50, The current logic calls get_current_repo_and_pr() and mixes its detected PR into a user-specified --repo, causing cross-repo corruption; change the behavior so that if the user passed --repo but not --pr you DO NOT substitute the detected PR from get_current_repo_and_pr() (only use detected PR when repo was not supplied), and when constructing GhCliAdapter bind it once with the final resolved repo_owner/repo_name (do not construct adapters that default back to the checkout repo); update the same pattern in the other affected blocks (the other get_current_repo_and_pr usages around the mentioned ranges) so repo selection is authoritative and the adapter is always created with that selected repo_owner/repo_name. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/publish.yml:
- Around line 8-10: The workflow currently grants id-token: write at the
workflow-level which gives the build job unnecessary OIDC token minting
capability; remove or change the top-level permissions so id-token: write is not
granted globally and instead add id-token: write only to the publish job (the
job that performs PyPI OIDC auth), leaving the build job with no id-token
permission (e.g., ensure build's permissions do not include id-token and only
publish job's permissions include id-token: write); update the permissions block
and the publish job's permissions accordingly.In
@CHANGELOG.md:
- Around line 7-21: The changelog fails markdownlint MD022 because the lists
under the headings### Addedand### Fixedare placed immediately after the
headings; insert a single blank line after each heading (i.e., after the### Addedand after the### Fixedlines) so the bullet lists are separated from
their headings and the file becomes lint-clean.In
@SECURITY.md:
- Around line 59-63: The Markdown headings "## Safe Harbor" and "## In Scope /
Out of Scope" are missing the required blank line after each heading; update the
SECURITY.md so there is a single blank line immediately following the "## Safe
Harbor" and the "## In Scope / Out of Scope" headings to satisfy the Markdown
linter (ensure no other content sits directly on the same line as the heading).In
@src/doghouse/adapters/git/git_adapter.py:
- Around line 40-47: The current handling in git_adapter.py treats any non-zero
unpushed_res.returncode from the git rev-list call as "local-no-upstream";
instead, inspect unpushed_res.stderr (and stdout if needed) and only map to
Blocker(id="local-no-upstream", BlockerType.LOCAL_UNPUSHED,
BlockerSeverity.WARNING) when the output contains a clear upstream-missing
message (e.g., contains phrases like "no upstream configured" or "no upstream"
for the branch); for any other non-zero result create a different blocker (e.g.,
LOCAL_GIT_ERROR) that includes the actual stderr text to preserve the real git
failure details and avoid misclassification while still using the existing
unpushed_res variable and Blocker construction pattern.In
@src/doghouse/cli/main.py:
- Around line 220-223: Validate the polling interval at the start of the watch
command and reject non-positive values: inside the watch(...) function check if
interval < 1 and raise a typer.BadParameter (or call typer.Exit after printing
an error) with a clear message like "interval must be >= 1" before entering the
polling loop; apply the same guard to the other polling loop referenced in the
review (the additional watch loop that polls GitHub/JSONL) so neither path can
enter a tight or invalid sleep.In
@src/doghouse/core/services/playback_service.py:
- Around line 21-27: The file-reading uses bare open() which is
locale-dependent; change the snapshot file reads to explicitly specify UTF-8
encoding when opening both current_path and baseline_path so json.load and
Snapshot.from_dict always decode using UTF-8 (update the open calls that wrap
json.load for current = Snapshot.from_dict(json.load(...)) and the baseline =
Snapshot.from_dict(json.load(...)) branch to pass encoding='utf-8').In
@src/doghouse/core/services/recorder_service.py:
- Around line 3-7: The NameError is caused because the dedupe code constructs
Blocker but recorder_service.py never imports it; add the proper import for the
Blocker class (e.g., from ..domain.blocker import Blocker) alongside the other
domain imports at the top of the module so Blocker is defined when
snapshot/dedupe logic runs; ensure any other references in this module to
Blocker (the duplicate blocker handling code) use that imported symbol.- Line 44: The merge logic in recorder_service.py currently compares
BlockerSeverity enum .value strings (b.severity and existing.severity)
lexicographically, causing wrong ordering (e.g., "warning" > "blocker"); replace
that comparison with an explicit severity ranking: define a severity_rank
mapping for BlockerSeverity members to numeric ranks and use
severity_rank[b.severity] > severity_rank[existing.severity] (or the inverse as
intended) to choose the higher severity when setting severity in the merge
expression that references b.severity and existing.severity.In
@tests/doghouse/test_delta_engine.py:
- Around line 55-117: Add a test for the "no prior baseline" path by calling
DeltaEngine.compute_delta(None, current) (use Snapshot to build current with a
few Blocker instances) and assert that delta.added_blockers contains all current
blockers while delta.removed_blockers and delta.still_open_blockers are empty;
name the test e.g. test_compute_delta_no_baseline and reference
DeltaEngine.compute_delta, Snapshot, and Blocker/BlockerType so the behavior for
a brand-new PR is covered.In
@tools/bootstrap-git-mind.sh:
- Around line 27-37: The bootstrap currently always writes the console entry
"git-mind = "git_mind.cli:run"" into pyproject.toml even when the package
sources under src/git_mind may be absent; update the script to check for the
presence of the package directory (src/git_mind) before emitting that entry and
either: (A) fail fast with a clear error and non-zero exit if src/git_mind is
missing, or (B) create a minimal stub module (git_mind/cli.py) with a no-op
run() function so the entry point is valid; apply the same check/behavior for
the similar code region referenced around lines 51-53 to avoid producing a dead
entry point.
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 9-11: Remove the unnecessary workflow permission "pull-requests:
write" from the permissions block so the CI token is not granted PR write
access; keep only "contents: read" (or the minimal permissions required) in the
permissions: section and ensure no job in this workflow needs higher privileges
before restoring any broader scope.In
@docs/archive/SPEC.md:
- Around line 5-1166: The file contains many unlabeled fenced code blocks
(MD040) and whitespace/newline hygiene issues starting at the "Title Screen" /
"0. Scroll View Widget" sections; fix by labeling all fenced blocks according to
their content (usetext for ASCII/UI art like the Title Screen and Scroll View examples,mermaid for diagrams, ```toml for the Config Structure block,
etc.), normalize the fence openings/closings so each block has matching
backticks, remove trailing spaces inside the file, and ensure exactly one
trailing newline at EOF; search for the unlabeled ASCII blocks, mermaid
diagrams, and the Config Structure sample to locate and update them.In
@docs/archive/SPRINTS.md:
- Around line 5-8: The "Cadence & Dates" section contains a stale start date
("Start date: Monday, 2025-11-10") that causes confusion; update the "Start
date" line under the Cadence & Dates header to either a current/upcoming sprint
start (e.g., compute and insert the next Monday date) or prepend an explicit
marker like "Historical:" / "Archive:" to the entire Cadence & Dates block
(including the "Sprint length" and "Code freeze" lines) so readers know these
are past sprint details rather than active planning info.- Around line 168-177: The Markdown headings "Backlog / Nice-to-Haves
(Post-SPEC)" and "Cross-Cutting Tech Debt & Risks" violate MD022 by not having a
blank line after the heading; fix by inserting a single blank line immediately
after each of those headings so the lists that follow are separated (preserve
existing list formatting and spacing otherwise) and rerun lint to confirm MD022
is resolved.In
@docs/FEATURES.md:
- Around line 242-346: The DP-US-0201 story block is nested under the wrong
feature heading (it's currently placed beneath the DP-F-21 Doghouse Flight
Recorder section); move the entire DP-US-0201 "Fetch and Render PR List"
subsection (including its User Story, Requirements, Acceptance Criteria, DoR,
and Test Plan) out from under the DP-F-21 section and insert it directly under
the DP-F-02 "Main Menu — PR Selection" heading so that DP-US-0201 is a child of
DP-F-02 and not DP-F-21, preserving the exact subsection content and markdown
heading levels.In
@PRODUCTION_LOG.mg:
- Around line 27-29: The "Initial Entries" empty-state banner and the "- (none
yet)" line are now inaccurate; remove the empty-state banner or replace it with
an actual summary of current log contents by editing the "Initial Entries"
header block and deleting the "- (none yet)" placeholder, or substitute it with
a brief, factual summary of the entries present.In
@README.md:
- Around line 140-149: Add blank lines around the headings and fenced code
blocks to satisfy markdownlint MD022/MD031: ensure there is a blank line before
and after the "### 📡 Capture a Sortie" and "### 🎬 Run a Playback" headings and
also a blank line before the openingbash and after the closingfor both
doghouse snapshotanddoghouse playback pb1_push_deltacode fences so the
README's structure is lint-clean while keeping the existing text unchanged.In
@src/doghouse/adapters/github/gh_cli_adapter.py:
- Around line 51-79: The current GraphQL query in gql_query and the logic that
assigns threads from gql_res only fetches the first page (reviewThreads(first:
100)) and loses threads on large PRs; update fetch_blockers() to paginate:
extend gql_query to request pageInfo { hasNextPage endCursor } and accept an
optional $after: String variable, then loop calling self._run_gh_json (passing
"-F", f"after={after}" each iteration) accumulating nodes into the threads list
until pageInfo.hasNextPage is false, updating after = endCursor each loop so all
reviewThreads are retrieved before downstream processing.In
@src/doghouse/adapters/storage/jsonl_adapter.py:
- Around line 26-41: The JSONL reader can be broken by a partial/torn append;
update save_snapshot and list_snapshots to be resilient: in save_snapshot
(function save_snapshot and helper _get_path) ensure each appended line is fully
flushed to disk (call f.flush() then os.fsync(f.fileno())) after writing
json.dumps(snapshot.to_dict()) + "\n" to reduce partial writes; in
list_snapshots (and any get_latest_snapshot that reads the same file) make the
per-line parsing robust by wrapping json.loads(...) and Snapshot.from_dict(...)
in a try/except catching JSONDecodeError/ValueError and other mapping errors,
skip malformed/empty lines, and optionally log a warning with the offending line
so a single bad line no longer bricks reads.In
@src/doghouse/cli/main.py:
- Around line 45-50: The current logic calls get_current_repo_and_pr() and mixes
its detected PR into a user-specified --repo, causing cross-repo corruption;
change the behavior so that if the user passed --repo but not --pr you DO NOT
substitute the detected PR from get_current_repo_and_pr() (only use detected PR
when repo was not supplied), and when constructing GhCliAdapter bind it once
with the final resolved repo_owner/repo_name (do not construct adapters that
default back to the checkout repo); update the same pattern in the other
affected blocks (the other get_current_repo_and_pr usages around the mentioned
ranges) so repo selection is authoritative and the adapter is always created
with that selected repo_owner/repo_name.In
@src/doghouse/core/domain/snapshot.py:
- Around line 13-53: The Snapshot still allows external mutation of nested
structures because post_init and to_dict/from_dict use shallow copies or
direct references; fix by using deep copies for snapshot-level metadata and
blocker.metadata in post_init (and import copy), ensure from_dict constructs
Blocker.metadata and Snapshot.metadata via copy.deepcopy of input dicts, and
have to_dict return deep-copied metadata/blocker.metadata so callers cannot
mutate internal state; reference post_init, to_dict, from_dict, Snapshot,
Blocker, and metadata/blockers when making the changes.In
@src/doghouse/core/services/delta_engine.py:
- Around line 13-20: The no-baseline branch of Delta construction currently
assigns current.blockers directly to Delta.added_blockers, which aliases the
same list; change the assignment in the Delta return in delta_engine.py to pass
a shallow copy of current.blockers (e.g., create a new list from
current.blockers) so mutations on Delta.added_blockers won’t affect
current.blockers; keep other fields (removed_blockers, still_open_blockers)
as-is.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: ASSERTIVE **Plan**: Pro **Run ID**: `f2e52bdf-2229-4bfa-a9a7-ab67a21ee5b7` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between cfcc3eef814c1369a838a3f3e63526335869fa22 and d4def9763b7168ff1970ce769b7b45be426bcc34. </details> <details> <summary>📒 Files selected for processing (27)</summary> * `.github/workflows/ci.yml` * `.github/workflows/publish.yml` * `CHANGELOG.md` * `Makefile` * `PRODUCTION_LOG.mg` * `README.md` * `SECURITY.md` * `docs/FEATURES.md` * `docs/archive/SPEC.md` * `docs/archive/SPRINTS.md` * `docs/code-reviews/PR5/6d8640d23be73ee61c9b962f90a4141768a3692f.md` * `doghouse/README.md` * `examples/8dfbfab49b290a969ed7bb6248f3880137ef177d.md` * `pyproject.toml` * `src/doghouse/adapters/git/git_adapter.py` * `src/doghouse/adapters/github/gh_cli_adapter.py` * `src/doghouse/adapters/storage/jsonl_adapter.py` * `src/doghouse/cli/main.py` * `src/doghouse/core/domain/delta.py` * `src/doghouse/core/domain/snapshot.py` * `src/doghouse/core/ports/github_port.py` * `src/doghouse/core/ports/storage_port.py` * `src/doghouse/core/services/delta_engine.py` * `src/doghouse/core/services/playback_service.py` * `src/doghouse/core/services/recorder_service.py` * `tests/doghouse/test_delta_engine.py` * `tools/bootstrap-git-mind.sh` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+8
to
+10
| permissions: | ||
| contents: read | ||
| id-token: write |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/publish.ymlRepository: flyingrobots/draft-punks
Length of output: 1113
Eliminate unnecessary OIDC token access in the build job.
The workflow-level id-token: write permission (lines 8-10) grants token-minting capability to the build job, which executes untrusted build logic (pip install, hatch build). The build job has zero legitimate need for OIDC tokens. Move id-token: write to the publish job only, as it's the sole consumer of PyPI OIDC authentication.
Scope the token to the publisher
permissions:
contents: read
- id-token: write
jobs:
build:
runs-on: ubuntu-latest
@@ -29,6 +29,8 @@ jobs:
publish:
needs: build
runs-on: ubuntu-latest
environment: pypi
+ permissions:
+ contents: read
+ id-token: write
steps:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/publish.yml around lines 8 - 10, The workflow currently
grants id-token: write at the workflow-level which gives the build job
unnecessary OIDC token minting capability; remove or change the top-level
permissions so id-token: write is not granted globally and instead add id-token:
write only to the publish job (the job that performs PyPI OIDC auth), leaving
the build job with no id-token permission (e.g., ensure build's permissions do
not include id-token and only publish job's permissions include id-token:
write); update the permissions block and the publish job's permissions
accordingly.
Comment on lines
+7
to
+21
| ### Added | ||
| - **Doghouse Flight Recorder**: A new agent-native engine for PR state reconstruction. | ||
| - **CLI Subcommands**: `snapshot`, `history`, `watch`, `playback`, `export`. | ||
| - **Blocking Matrix**: Logic to distinguish Primary (conflicts) from Secondary (stale checks) blockers. | ||
| - **Local Awareness**: Detection of uncommitted/unpushed local repository state. | ||
| - **Machine-Readable Output**: `--json` flag for all major commands to support Thinking Automatons. | ||
| - **Repro Bundles**: `export` command to create "Manuscript Fragments" for debugging. | ||
|
|
||
| ### Fixed | ||
| - **CI/CD Security**: Added top-level permissions to workflows and expanded branch scope. | ||
| - **Publishing Hygiene**: Refined tag patterns and split build/publish steps. | ||
| - **Core Immutability**: Ensure Snapshot and Blocker objects own immutable copies of data. | ||
| - **Deterministic Delta**: Sorted blocker IDs to ensure stable output across runs. | ||
| - **Error Handling**: Hardened subprocess calls with timeouts and missing-upstream detection. | ||
| - **Import Paths**: Fixed packaging bugs identified via recursive dogfooding. |
Contributor
There was a problem hiding this comment.
Markdownlint still fails under these subheadings.
The ### Added and ### Fixed lists start immediately after the headings, so MD022 is still firing here. Add a blank line under each heading and keep the changelog lint-clean.
Patch
### Added
+
- **Doghouse Flight Recorder**: A new agent-native engine for PR state reconstruction.
- **CLI Subcommands**: `snapshot`, `history`, `watch`, `playback`, `export`.
- **Blocking Matrix**: Logic to distinguish Primary (conflicts) from Secondary (stale checks) blockers.
@@
### Fixed
+
- **CI/CD Security**: Added top-level permissions to workflows and expanded branch scope.
- **Publishing Hygiene**: Refined tag patterns and split build/publish steps.
- **Core Immutability**: Ensure Snapshot and Blocker objects own immutable copies of data.📝 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
| ### Added | |
| - **Doghouse Flight Recorder**: A new agent-native engine for PR state reconstruction. | |
| - **CLI Subcommands**: `snapshot`, `history`, `watch`, `playback`, `export`. | |
| - **Blocking Matrix**: Logic to distinguish Primary (conflicts) from Secondary (stale checks) blockers. | |
| - **Local Awareness**: Detection of uncommitted/unpushed local repository state. | |
| - **Machine-Readable Output**: `--json` flag for all major commands to support Thinking Automatons. | |
| - **Repro Bundles**: `export` command to create "Manuscript Fragments" for debugging. | |
| ### Fixed | |
| - **CI/CD Security**: Added top-level permissions to workflows and expanded branch scope. | |
| - **Publishing Hygiene**: Refined tag patterns and split build/publish steps. | |
| - **Core Immutability**: Ensure Snapshot and Blocker objects own immutable copies of data. | |
| - **Deterministic Delta**: Sorted blocker IDs to ensure stable output across runs. | |
| - **Error Handling**: Hardened subprocess calls with timeouts and missing-upstream detection. | |
| - **Import Paths**: Fixed packaging bugs identified via recursive dogfooding. | |
| ### Added | |
| - **Doghouse Flight Recorder**: A new agent-native engine for PR state reconstruction. | |
| - **CLI Subcommands**: `snapshot`, `history`, `watch`, `playback`, `export`. | |
| - **Blocking Matrix**: Logic to distinguish Primary (conflicts) from Secondary (stale checks) blockers. | |
| - **Local Awareness**: Detection of uncommitted/unpushed local repository state. | |
| - **Machine-Readable Output**: `--json` flag for all major commands to support Thinking Automatons. | |
| - **Repro Bundles**: `export` command to create "Manuscript Fragments" for debugging. | |
| ### Fixed | |
| - **CI/CD Security**: Added top-level permissions to workflows and expanded branch scope. | |
| - **Publishing Hygiene**: Refined tag patterns and split build/publish steps. | |
| - **Core Immutability**: Ensure Snapshot and Blocker objects own immutable copies of data. | |
| - **Deterministic Delta**: Sorted blocker IDs to ensure stable output across runs. | |
| - **Error Handling**: Hardened subprocess calls with timeouts and missing-upstream detection. | |
| - **Import Paths**: Fixed packaging bugs identified via recursive dogfooding. |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 7-7: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 15-15: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` around lines 7 - 21, The changelog fails markdownlint MD022
because the lists under the headings `### Added` and `### Fixed` are placed
immediately after the headings; insert a single blank line after each heading
(i.e., after the `### Added` and after the `### Fixed` lines) so the bullet
lists are separated from their headings and the file becomes lint-clean.
Comment on lines
59
to
+63
| ## Safe Harbor | ||
| If you make a good-faith effort to comply with this policy, we will not pursue civil or criminal action. Do not access user data, pivot laterally, persist, or degrade availability. Limit testing to your own accounts. | ||
|
|
||
| ## In Scope / Out of Scope | ||
| - In scope: vulnerabilities affecting supported versions and first-party services. | ||
| - In scope: vulnerabilities affecting supported versions and first-party services. |
Contributor
There was a problem hiding this comment.
Add missing blank lines after headings to satisfy Markdown lint.
## Safe Harbor and ## In Scope / Out of Scope are missing the required blank line below the heading, which can break lint-gated docs checks.
🛠️ Minimal fix
## Safe Harbor
+
If you make a good-faith effort to comply with this policy, we will not pursue civil or criminal action. Do not access user data, pivot laterally, persist, or degrade availability. Limit testing to your own accounts.
## In Scope / Out of Scope
+
- In scope: vulnerabilities affecting supported versions and first-party services.📝 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
| ## Safe Harbor | |
| If you make a good-faith effort to comply with this policy, we will not pursue civil or criminal action. Do not access user data, pivot laterally, persist, or degrade availability. Limit testing to your own accounts. | |
| ## In Scope / Out of Scope | |
| - In scope: vulnerabilities affecting supported versions and first-party services. | |
| - In scope: vulnerabilities affecting supported versions and first-party services. | |
| ## Safe Harbor | |
| If you make a good-faith effort to comply with this policy, we will not pursue civil or criminal action. Do not access user data, pivot laterally, persist, or degrade availability. Limit testing to your own accounts. | |
| ## In Scope / Out of Scope | |
| - In scope: vulnerabilities affecting supported versions and first-party services. |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 59-59: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 62-62: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SECURITY.md` around lines 59 - 63, The Markdown headings "## Safe Harbor" and
"## In Scope / Out of Scope" are missing the required blank line after each
heading; update the SECURITY.md so there is a single blank line immediately
following the "## Safe Harbor" and the "## In Scope / Out of Scope" headings to
satisfy the Markdown linter (ensure no other content sits directly on the same
line as the heading).
Comment on lines
+40
to
+47
| elif unpushed_res.returncode != 0: | ||
| # Upstream might be missing | ||
| blockers.append(Blocker( | ||
| id="local-no-upstream", | ||
| type=BlockerType.LOCAL_UNPUSHED, | ||
| message="Local branch has no upstream configured", | ||
| severity=BlockerSeverity.WARNING | ||
| )) |
Contributor
There was a problem hiding this comment.
Do not map every git rev-list failure to “no upstream configured.”
This branch currently misclassifies all failures as missing upstream. That can produce wrong blocker messages and hide real local git failures.
🔧 Suggested fix
if unpushed_res.returncode == 0 and unpushed_res.stdout.strip():
count = len(unpushed_res.stdout.strip().split("\n"))
blockers.append(Blocker(
id="local-unpushed",
type=BlockerType.LOCAL_UNPUSHED,
message=f"Local branch is ahead of remote by {count} commits",
severity=BlockerSeverity.WARNING
))
- elif unpushed_res.returncode != 0:
- # Upstream might be missing
+ elif unpushed_res.returncode != 0 and "no upstream" in unpushed_res.stderr.lower():
blockers.append(Blocker(
id="local-no-upstream",
type=BlockerType.LOCAL_UNPUSHED,
message="Local branch has no upstream configured",
severity=BlockerSeverity.WARNING
))
+ elif unpushed_res.returncode != 0:
+ blockers.append(Blocker(
+ id="local-git-state-unknown",
+ type=BlockerType.OTHER,
+ message="Unable to determine unpushed commits (git command failed)",
+ severity=BlockerSeverity.INFO
+ ))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/doghouse/adapters/git/git_adapter.py` around lines 40 - 47, The current
handling in git_adapter.py treats any non-zero unpushed_res.returncode from the
git rev-list call as "local-no-upstream"; instead, inspect unpushed_res.stderr
(and stdout if needed) and only map to Blocker(id="local-no-upstream",
BlockerType.LOCAL_UNPUSHED, BlockerSeverity.WARNING) when the output contains a
clear upstream-missing message (e.g., contains phrases like "no upstream
configured" or "no upstream" for the branch); for any other non-zero result
create a different blocker (e.g., LOCAL_GIT_ERROR) that includes the actual
stderr text to preserve the real git failure details and avoid misclassification
while still using the existing unpushed_res variable and Blocker construction
pattern.
Comment on lines
+220
to
+223
| def watch( | ||
| pr: Optional[int] = typer.Option(None, "--pr", help="PR number"), | ||
| repo: Optional[str] = typer.Option(None, "--repo", help="Repository (owner/name)"), | ||
| interval: int = typer.Option(180, "--interval", help="Polling interval in seconds") |
Contributor
There was a problem hiding this comment.
Reject non-positive watch intervals.
time.sleep(0) turns this into a tight poll loop that will hammer GitHub and the JSONL store; negative values blow up on the first sleep. Guard interval >= 1 before entering the loop.
Patch
def watch(
pr: Optional[int] = typer.Option(None, "--pr", help="PR number"),
repo: Optional[str] = typer.Option(None, "--repo", help="Repository (owner/name)"),
interval: int = typer.Option(180, "--interval", help="Polling interval in seconds")
):
"""PhiedBach's Radar: Live monitoring of PR state."""
+ if interval < 1:
+ console.print("[red]Error: --interval must be >= 1[/red]")
+ raise typer.Exit(2)
+
if not repo or not pr:
detected_repo, detected_pr = get_current_repo_and_pr()
repo = repo or detected_repo
pr = pr or detected_prAlso applies to: 239-265
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/doghouse/cli/main.py` around lines 220 - 223, Validate the polling
interval at the start of the watch command and reject non-positive values:
inside the watch(...) function check if interval < 1 and raise a
typer.BadParameter (or call typer.Exit after printing an error) with a clear
message like "interval must be >= 1" before entering the polling loop; apply the
same guard to the other polling loop referenced in the review (the additional
watch loop that polls GitHub/JSONL) so neither path can enter a tight or invalid
sleep.
Comment on lines
+21
to
+27
| with open(current_path) as f: | ||
| current = Snapshot.from_dict(json.load(f)) | ||
|
|
||
| baseline = None | ||
| if baseline_path.exists(): | ||
| with open(baseline_path) as f: | ||
| baseline = Snapshot.from_dict(json.load(f)) |
Contributor
There was a problem hiding this comment.
Lock fixture decoding to UTF-8.
Bare open() makes replay depend on the host locale. These snapshots can carry non-ASCII PR metadata or comments, so a non-UTF-8 environment can fail or decode differently than CI.
Patch
- with open(current_path) as f:
+ with open(current_path, encoding="utf-8") as f:
current = Snapshot.from_dict(json.load(f))
@@
- with open(baseline_path) as f:
+ with open(baseline_path, encoding="utf-8") as f:
baseline = Snapshot.from_dict(json.load(f))📝 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
| with open(current_path) as f: | |
| current = Snapshot.from_dict(json.load(f)) | |
| baseline = None | |
| if baseline_path.exists(): | |
| with open(baseline_path) as f: | |
| baseline = Snapshot.from_dict(json.load(f)) | |
| with open(current_path, encoding="utf-8") as f: | |
| current = Snapshot.from_dict(json.load(f)) | |
| baseline = None | |
| if baseline_path.exists(): | |
| with open(baseline_path, encoding="utf-8") as f: | |
| baseline = Snapshot.from_dict(json.load(f)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/doghouse/core/services/playback_service.py` around lines 21 - 27, The
file-reading uses bare open() which is locale-dependent; change the snapshot
file reads to explicitly specify UTF-8 encoding when opening both current_path
and baseline_path so json.load and Snapshot.from_dict always decode using UTF-8
(update the open calls that wrap json.load for current =
Snapshot.from_dict(json.load(...)) and the baseline =
Snapshot.from_dict(json.load(...)) branch to pass encoding='utf-8').
Comment on lines
+3
to
+7
| from ..domain.snapshot import Snapshot | ||
| from ..domain.delta import Delta | ||
| from ..ports.github_port import GitHubPort | ||
| from ..ports.storage_port import StoragePort | ||
| from .delta_engine import DeltaEngine |
Contributor
There was a problem hiding this comment.
This merge path throws NameError on the first duplicate blocker.
The dedupe branch constructs Blocker(...), but Blocker is never imported in this module. As soon as local and remote sources share an ID, snapshotting blows up.
Patch
from ..domain.snapshot import Snapshot
from ..domain.delta import Delta
+from ..domain.blocker import Blocker
from ..ports.github_port import GitHubPort
from ..ports.storage_port import StoragePort
from .delta_engine import DeltaEngineAlso applies to: 40-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/doghouse/core/services/recorder_service.py` around lines 3 - 7, The
NameError is caused because the dedupe code constructs Blocker but
recorder_service.py never imports it; add the proper import for the Blocker
class (e.g., from ..domain.blocker import Blocker) alongside the other domain
imports at the top of the module so Blocker is defined when snapshot/dedupe
logic runs; ensure any other references in this module to Blocker (the duplicate
blocker handling code) use that imported symbol.
| id=b.id, | ||
| type=b.type, | ||
| message=b.message, | ||
| severity=b.severity if b.severity.value > existing.severity.value else existing.severity, |
Contributor
There was a problem hiding this comment.
String ordering makes warning beat blocker.
BlockerSeverity is a plain string-valued enum. Comparing .value here is lexicographic, so "warning" currently outranks "blocker" and a merged blocker can be downgraded incorrectly. Use an explicit severity rank.
Patch
+ severity_rank = {"info": 0, "warning": 1, "blocker": 2}
blocker_map = {b.id: b for b in remote_blockers}
for b in local_blockers:
if b.id in blocker_map:
# Merge logic: if either is primary, it stays primary
existing = blocker_map[b.id]
blocker_map[b.id] = Blocker(
id=b.id,
type=b.type,
message=b.message,
- severity=b.severity if b.severity.value > existing.severity.value else existing.severity,
+ severity=(
+ b.severity
+ if severity_rank[b.severity.value] > severity_rank[existing.severity.value]
+ else existing.severity
+ ),
is_primary=b.is_primary or existing.is_primary,
metadata={**existing.metadata, **b.metadata}
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/doghouse/core/services/recorder_service.py` at line 44, The merge logic
in recorder_service.py currently compares BlockerSeverity enum .value strings
(b.severity and existing.severity) lexicographically, causing wrong ordering
(e.g., "warning" > "blocker"); replace that comparison with an explicit severity
ranking: define a severity_rank mapping for BlockerSeverity members to numeric
ranks and use severity_rank[b.severity] > severity_rank[existing.severity] (or
the inverse as intended) to choose the higher severity when setting severity in
the merge expression that references b.severity and existing.severity.
Comment on lines
+55
to
+117
| def test_compute_delta_empty_blockers(): | ||
| engine = DeltaEngine() | ||
| baseline = Snapshot( | ||
| timestamp=datetime.datetime(2026, 1, 1, tzinfo=datetime.timezone.utc), | ||
| head_sha="sha1", | ||
| blockers=[] | ||
| ) | ||
| current = Snapshot( | ||
| timestamp=datetime.datetime(2026, 1, 2, tzinfo=datetime.timezone.utc), | ||
| head_sha="sha1", | ||
| blockers=[] | ||
| ) | ||
| delta = engine.compute_delta(baseline, current) | ||
| assert len(delta.added_blockers) == 0 | ||
| assert len(delta.removed_blockers) == 0 | ||
| assert len(delta.still_open_blockers) == 0 | ||
|
|
||
| def test_compute_delta_overlapping_blockers(): | ||
| engine = DeltaEngine() | ||
| b1 = Blocker(id="1", type=BlockerType.UNRESOLVED_THREAD, message="msg1") | ||
| b2 = Blocker(id="2", type=BlockerType.UNRESOLVED_THREAD, message="msg2") | ||
| b3 = Blocker(id="3", type=BlockerType.UNRESOLVED_THREAD, message="msg3") | ||
|
|
||
| baseline = Snapshot( | ||
| timestamp=datetime.datetime(2026, 1, 1, tzinfo=datetime.timezone.utc), | ||
| head_sha="sha1", | ||
| blockers=[b1, b2] | ||
| ) | ||
| current = Snapshot( | ||
| timestamp=datetime.datetime(2026, 1, 2, tzinfo=datetime.timezone.utc), | ||
| head_sha="sha1", | ||
| blockers=[b2, b3] | ||
| ) | ||
|
|
||
| delta = engine.compute_delta(baseline, current) | ||
| assert len(delta.added_blockers) == 1 | ||
| assert delta.added_blockers[0].id == "3" | ||
| assert len(delta.removed_blockers) == 1 | ||
| assert delta.removed_blockers[0].id == "1" | ||
| assert len(delta.still_open_blockers) == 1 | ||
| assert delta.still_open_blockers[0].id == "2" | ||
|
|
||
| def test_compute_delta_mutated_blocker(): | ||
| # If ID is same but content changes, it's still "still_open" in current logic | ||
| # because ID is the primary key for delta. | ||
| engine = DeltaEngine() | ||
| b1_v1 = Blocker(id="1", type=BlockerType.UNRESOLVED_THREAD, message="msg1") | ||
| b1_v2 = Blocker(id="1", type=BlockerType.UNRESOLVED_THREAD, message="msg1-updated") | ||
|
|
||
| baseline = Snapshot( | ||
| timestamp=datetime.datetime(2026, 1, 1, tzinfo=datetime.timezone.utc), | ||
| head_sha="sha1", | ||
| blockers=[b1_v1] | ||
| ) | ||
| current = Snapshot( | ||
| timestamp=datetime.datetime(2026, 1, 2, tzinfo=datetime.timezone.utc), | ||
| head_sha="sha1", | ||
| blockers=[b1_v2] | ||
| ) | ||
|
|
||
| delta = engine.compute_delta(baseline, current) | ||
| assert len(delta.still_open_blockers) == 1 | ||
| assert delta.still_open_blockers[0].message == "msg1-updated" |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add the no-baseline case.
You covered blocker-set diffs, but not the first-sortie path. snapshot/watch both have explicit “no prior baseline” behavior, and this suite never pins DeltaEngine.compute_delta(None, current). That is the production path on a brand-new PR, so please lock down the expected “all current blockers are added / no baseline SHA” semantics here.
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 58-58: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
[warning] 63-63: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
[warning] 79-79: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
[warning] 84-84: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
[warning] 105-105: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
[warning] 110-110: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/doghouse/test_delta_engine.py` around lines 55 - 117, Add a test for
the "no prior baseline" path by calling DeltaEngine.compute_delta(None, current)
(use Snapshot to build current with a few Blocker instances) and assert that
delta.added_blockers contains all current blockers while delta.removed_blockers
and delta.still_open_blockers are empty; name the test e.g.
test_compute_delta_no_baseline and reference DeltaEngine.compute_delta,
Snapshot, and Blocker/BlockerType so the behavior for a brand-new PR is covered.
Comment on lines
+27
to
+37
| cat >"$DEST/pyproject.toml" <<'PY' | ||
| [project] | ||
| name = "git-mind" | ||
| version = "0.0.1" | ||
| description = "Git-native conversational ops: sessions as refs, commits as speech-acts." | ||
| authors = [{name = "GATOS"}] | ||
| requires-python = ">=3.11" | ||
| dependencies = ["typer>=0.12"] | ||
|
|
||
| [project.scripts] | ||
| git-mind = "git_mind.cli:run" |
Contributor
There was a problem hiding this comment.
Bootstrap can generate a dead entry point.
The script always writes git-mind = "git_mind.cli:run", but a few lines later it treats src/git_mind as optional. When those sources are missing, bootstrap still succeeds and emits a package whose console entry cannot import. Fail fast there or generate a stub git_mind/cli.py.
Also applies to: 51-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/bootstrap-git-mind.sh` around lines 27 - 37, The bootstrap currently
always writes the console entry "git-mind = \"git_mind.cli:run\"" into
pyproject.toml even when the package sources under src/git_mind may be absent;
update the script to check for the presence of the package directory
(src/git_mind) before emitting that entry and either: (A) fail fast with a clear
error and non-zero exit if src/git_mind is missing, or (B) create a minimal stub
module (git_mind/cli.py) with a no-op run() function so the entry point is
valid; apply the same check/behavior for the similar code region referenced
around lines 51-53 to avoid producing a dead entry point.
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
1 participant
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.
🎼 Ze Doghouse has Arrived!\n\nThis PR reboots the project to focus on DOGHOUSE, the agent-native flight recorder for PR sorties. We have cleared the legacy Draft Punks TUI and GATOS/git-mind drift to establish a clean, hexagonal foundation.\n\n### 🐕 Key Accomplishments:\n- Core Engine: Snapshot, Blocker, and Delta models for state reconstruction.\n- Radar: A new
watchcommand for live PR monitoring.\n- Black Box: Anexportcommand for creating 'Manuscript Fragment' repro bundles.\n- Blocking Matrix: Semantic logic to distinguish Primary (conflicts) from Secondary (stale checks) blockers.\n- Local Awareness: Detection of 'mid-maneuver' uncommitted/unpushed state.\n- Agent-Native: Built-in--jsonoutput and JSONL storage for ze 'Thinking Automatons'.\n\n### 🎹 Ze Lore & Flavor:\n- PhiedBach and BunBun are back at the podium.\n- Snoopy is in the cockpit, dogfighting ze Red Baron.\n- Spectacles are sliding, Red Bulls are hissing, and wicker baskets are being gnawed.\n\n### 🎬 Playbacks:\nSeededpb1_push_deltaandpb2_merge_readyto verify the engine deterministically.\n\n*“Every PR is a flight. Doghouse is the black box.”*\n\nWOOF! 🎹🐕🐇☁️✨🤖