(adr-0001) Parse one GNU Makefile into versioned JSON facts - #6
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary
WalkthroughThe pull request replaces the placeholder crate with a GNU Makefile parser that emits schema-v1 JSON, supports file and stdin input, preserves source locations, reports recovery and fatal errors, and adds parser, CLI, schema, snapshot, BDD, end-to-end, documentation, and tooling coverage. ChangesGNU Makefile parsing
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant SourceReader
participant MakefileLosslessParser
participant parse_source
User->>CLI: makeutil parse input
CLI->>SourceReader: read path or stdin
SourceReader-->>CLI: source bytes
CLI->>MakefileLosslessParser: parse UTF-8 source
MakefileLosslessParser-->>parse_source: syntax observations
parse_source-->>CLI: schema-v1 ParseReport
CLI-->>User: newline-terminated JSON and exit code
Possibly related PRs
Suggested labels: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 warnings, 2 inconclusive)
✅ Passed checks (15 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai resume |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== src/adapters/source.rs ==="
cat -n src/adapters/source.rs
echo "=== src/adapters/cli.rs ==="
cat -n src/adapters/cli.rs
echo "=== git log for these files ==="
git log --oneline -- src/adapters/source.rs src/adapters/cli.rs | head -20Length of output: 13699
No outstanding work remains from this list; no follow-up issue is needed. (੭ ˘ ᵕ˘)੭ nice one, all green now |
✅ Action performedReviews resumed. |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
@coderabbitai review |
✅ Action performedReview finished.
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@coderabbitai the library surface is for testing only |
|
Tip For best results, initiate chat on the files or code changes.
(=^･ω･^=) ✧ ✏️ Learnings added
|
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. (Treat documentation and validation coverage as in scope). ❌ Failed checks (2 warnings)
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
This comment was marked as resolved.
This comment was marked as resolved.
03df0e0 to
381edd4
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="leynos/agent-template-python"
title="Replace the undeclared mbake validation command"
body="$(cat <<'EOF'
## Summary
Replace the undeclared `mbake` validation command referenced by repository documentation or validation guidance.
## Rationale
The repository `Makefile` is the public validation entrypoint and exposes the supported project commands. It does not declare an `mbake` target or provide an installation/provisioning path for an `mbake` executable. Documentation validation must use reproducible project commands.
## Required changes
- Replace `mbake` with an appropriate declared `make` target; **or**
- Document and provision `mbake`, including a reproducible installation path and its relationship to the repository validation workflow.
- Keep documentation and validation instructions aligned with the repository's public `Makefile` entrypoints.
- Update validation coverage or documentation checks where needed to prevent references to undeclared validation commands.
## Acceptance criteria
- Every documented validation command is either a declared `Makefile` target or has explicit, reproducible provisioning documentation.
- No unsupported `mbake` invocation remains unless `mbake` is documented and provisioned by the project.
- Documentation validation uses reproducible project commands.
## Triage
`[type:docstyle]`
## Backlinks
- Requester: `@leynos`
- Raised during review of [leynos/makeutil#6](https://github.com/leynos/makeutil/pull/6).
- Source request: https://github.com/leynos/makeutil/pull/6#issuecomment-0
EOF
)"
issue_url="$(gh issue create \
--repo "$repo" \
--title "$title" \
--body "$body" \
--assignee leynos)"
printf '%s\n' "$issue_url"Length of output: 1928 🏁 Script executed: #!/bin/bash
set -euo pipefail
pr_repo="leynos/makeutil"
target_repo="leynos/agent-template-python"
issue_number="46"
comment_url="$(
gh api "repos/${pr_repo}/issues/6/comments?per_page=100" --paginate \
--jq '.[] | select(.user.login == "leynos" and (.body | contains("against `leynos/agent-template-python`")) and (.body | contains("undeclared `mbake`"))) | .html_url' \
| tail -n 1
)"
if [ -z "$comment_url" ]; then
echo "Could not locate the exact source comment permalink; leaving the issue unchanged." >&2
exit 1
fi
body="$(cat <<EOF
## Summary
Replace the undeclared \`mbake\` validation command referenced by repository documentation or validation guidance.
## Rationale
The repository \`Makefile\` is the public validation entrypoint and exposes the supported project commands. It does not declare an \`mbake\` target or provide an installation/provisioning path for an \`mbake\` executable. Documentation validation must use reproducible project commands.
## Required changes
- Replace \`mbake\` with an appropriate declared \`make\` target; **or**
- Document and provision \`mbake\`, including a reproducible installation path and its relationship to the repository validation workflow.
- Keep documentation and validation instructions aligned with the repository's public \`Makefile\` entrypoints.
- Update validation coverage or documentation checks where needed to prevent references to undeclared validation commands.
## Acceptance criteria
- Every documented validation command is either a declared \`Makefile\` target or has explicit, reproducible provisioning documentation.
- No unsupported \`mbake\` invocation remains unless \`mbake\` is documented and provisioned by the project.
- Documentation validation uses reproducible project commands.
## Triage
\`[type:docstyle]\`
## Backlinks
- Requester: `@leynos`
- Raised during review of [leynos/makeutil#6](https://github.com/leynos/makeutil/pull/6).
- [Source request](${comment_url})
EOF
)"
gh issue edit "$issue_number" --repo "$target_repo" --body "$body"
printf 'Updated https://github.com/%s/issues/%s\n' "$target_repo" "$issue_number"Length of output: 2399
The issue includes the required remediation, rationale, acceptance criteria, |
Resolve only source lines referenced by fallback diagnostics so newline-heavy input cannot allocate one span per physical line. Preserve diagnostic order, span calculation, and end-of-source fallback behaviour. Expose reproducible Makefile validation through make, correct the imported upstream links and repository layout, and restore single-line table rows in the vendored guide.
|
@coderabbitai Have the following now been resolved? docs/rstest-bdd-users-guide.md (1)
continuation lines as separate rows, so the vendored and published API mappings are misaligned. standard GitHub-flavoured Markdown table syntax. 🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In Sources: Coding guidelines, Path instructions |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Target table region ==='
sed -n '1078,1110p' docs/rstest-bdd-users-guide.md
echo
echo '=== Numbered target table region ==='
nl -ba docs/rstest-bdd-users-guide.md | sed -n '1080,1105p'
echo
echo '=== Markdown table-row shape check ==='
python3 - <<'PY'
from pathlib import Path
path = Path("docs/rstest-bdd-users-guide.md")
lines = path.read_text(encoding="utf-8").splitlines()
start, end = 1080, 1105
for number in range(start, end + 1):
line = lines[number - 1]
if line.strip():
print(f"{number}: pipes={line.count('|')} starts_pipe={line.startswith('|')} {line}")
PYLength of output: 3021
Keep the current table unchanged. In The line-number utility was unavailable in the sandbox, but direct inspection of the target section confirmed there are no wrapped cells or continuation rows. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Name the checked-in `cli_e2e` integration target so the documented focused test command is directly runnable.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 91-92: Update the validate-makefile target to provision and invoke
a pinned mbake version rather than relying on an unqualified system binary. Add
the required version to the repository’s dependency or tool configuration and
document that version alongside the validate-makefile target, preserving its
existing validation behavior.
🪄 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 Plus
Run ID: 017af915-99c1-404f-a3ce-69041d9e0080
📒 Files selected for processing (7)
Makefiledocs/developers-guide.mddocs/execplans/adr-0001-single-file-gnu-make-parse.mddocs/ortho-config-users-guide.mddocs/repository-layout.mddocs/rstest-bdd-users-guide.mdsrc/adapters/makefile.rs
|
@coderabbitai Have the following now been resolved? docs/execplans/adr-0001-single-file-gnu-make-parse.md (1)
🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In |
|
@coderabbitai Have the following now been resolved? docs/rstest-bdd-users-guide.md (1)
continuation lines as separate rows, so the vendored and published API mappings are misaligned. standard GitHub-flavoured Markdown table syntax. 🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In Sources: Coding guidelines, Path instructions |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Target section ==='
sed -n '1080,1108p' docs/rstest-bdd-users-guide.md
echo
echo '=== Numbered physical lines ==='
nl -ba docs/rstest-bdd-users-guide.md | sed -n '1084,1104p'
echo
echo '=== Table-row shape ==='
python3 - <<'PY'
from pathlib import Path
path = Path("docs/rstest-bdd-users-guide.md")
lines = path.read_text(encoding="utf-8").splitlines()
for number in range(1088, 1101):
line = lines[number - 1]
print(f"{number}: starts_pipe={line.startswith('|')} pipes={line.count('|')} :: {line}")
PYLength of output: 2738
Keep the table unchanged. The target section contains one physical line for the header, separator, and each of the four comparison rows. Each row uses standard GitHub-flavoured Markdown table syntax. The escaped The sandbox does not provide You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== ExecPlan integration-test commands ==='
rg -n -C 3 --fixed-strings \
-e 'cargo test --test parse_cli' \
-e 'cargo test --test cli_e2e --all-features' \
docs/execplans/adr-0001-single-file-gnu-make-parse.md || true
echo
echo '=== Matching integration-test files ==='
fd -t f -d 2 '^(cli_e2e|parse_cli)\.rs$' tests || true
echo
echo '=== All stale parse_cli references in the ExecPlan ==='
rg -n -C 2 --fixed-strings 'parse_cli' \
docs/execplans/adr-0001-single-file-gnu-make-parse.md || trueLength of output: 622
Keep the finding unresolved until the retry confirms both conditions:
You are interacting with an AI system. |
Escape control characters in fatal diagnostics so caller-controlled paths cannot forge stderr lines. Expand black-box coverage for inert Make constructs, include non-traversal, exact path identity, and configuration isolation. Replace recursive conditional collection with an iterative event stack and prove the 256-level resource guardrail. Add the 0.2.0 migration guide and document the Makefile validation tooling.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design.md`:
- Around line 462-464: Add the missing comma before “so” in the
control-character escaping sentence at docs/design.md lines 462-464,
docs/users-guide.md lines 55-56, and docs/developers-guide.md lines 87-91,
preserving the existing wording and behavior.
In `@docs/v0-2-0-migration-guide.md`:
- Around line 34-37: Reword the exit-status description in the migration guide
so the statuses `0`, `1`, and `2` are described in one sentence with a shared
structure, avoiding three consecutive sentences that begin with “Exit status.”
Preserve the existing meanings for each status and the schema_version
requirement.
🪄 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 Plus
Run ID: 7a103cdb-2ef4-4ba8-91c5-8295a37e01be
📒 Files selected for processing (10)
docs/contents.mddocs/design.mddocs/developers-guide.mddocs/execplans/adr-0001-single-file-gnu-make-parse.mddocs/users-guide.mddocs/v0-2-0-migration-guide.mdsrc/adapters/cli.rssrc/adapters/makefile.rssrc/adapters/makefile_tests.rstests/cli_e2e.rs
|
@coderabbitai please create new GitHub issues for the following: ❌ Failed checks (2 warnings)
|
Add the missing punctuation to the control-escaping contract and describe all migration exit statuses in one parallel sentence.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="leynos/makeutil"
pr_number="6"
echo "=== Existing open issues with matching terms ==="
gh issue list --repo "$repo" --state open --limit 100 \
--search 'in:title,body (serde OR parse_source OR performance OR nesting OR output growth)' \
--json number,title,url \
--jq '.[] | "\(.number)\t\(.title)\t\(.url)"' || true
echo "=== Source comment permalink ==="
gh api "repos/${repo}/issues/${pr_number}/comments?per_page=100" --paginate \
--jq '.[] | select(.user.login == "leynos" and (.body | contains("src/domain") and contains("public parse_source accepts unbounded bytes"))) | .html_url' \
| tail -n 1Length of output: 872 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="leynos/makeutil"
pr_url="https://github.com/leynos/makeutil/pull/6"
requester="leynos"
domain_body="$(cat <<EOF
## Summary
Separate the pure parsing domain model from JSON serialisation and parser/tool identity metadata.
## Rationale
The current \`src/domain\` types derive \`serde::Serialize\` and use JSON-specific serde renames. \`ParseReport\` also embeds parser identity. These format and vendor details couple the domain model to one output adapter.
## Affected areas
- \`src/domain/\`
- \`src/application.rs\`
- JSON output code in \`src/adapters/\`
- Report-schema and consumer compatibility tests
## Required changes
- Remove JSON/Serde annotations from pure domain facts.
- Move JSON field names and serialisation behaviour into adapter-owned DTOs.
- Move parser and tool identity metadata out of \`ParseReport\` and into the output DTO or output adapter.
- Map domain facts to the versioned JSON DTO before serialisation.
- Preserve the existing schema-v1 JSON contract unless an explicit schema migration is introduced.
- Update documentation and tests for the new boundary.
## Acceptance criteria
- Domain types do not import or derive serde serialisation traits.
- Domain types do not contain JSON-specific rename attributes.
- Parser and tool identity metadata is owned by the output boundary, not by pure domain facts.
- The JSON adapter maps domain facts to a versioned DTO.
- Schema validation, consumer-shaped deserialisation, and existing report behaviour remain covered by tests.
- Documentation describes the revised domain and adapter ownership.
## Backlinks
- Originating PR: ${pr_url}
- Source request: The GitHub API did not expose a permalink for the request comment at issue creation time.
- Requester: @${requester}
EOF
)"
performance_body="$(cat <<EOF
## Summary
Bound public parsing resource use and add regression coverage for large input, deep conditional nesting, and output growth.
## Rationale
The CLI limits source reads to 16 MiB, but public \`parse_source\` accepts unbounded byte slices. Nested facts clone full condition ancestry. The repository has no realistic 10 MiB regression or output-growth measurement to prevent resource regressions.
## Affected areas
- \`src/application.rs\`
- \`src/domain/\`
- \`src/adapters/source.rs\`
- \`src/adapters/makefile.rs\`
- Parser and integration test modules
- Resource-limit documentation
## Required changes
- Enforce a source-size bound at the public parsing boundary, or define and enforce equivalent bounds for condition ancestry and report/output growth.
- Keep CLI and library behaviour consistent for bounded input.
- Reduce or cap repeated condition-ancestry allocation where required by the selected limit.
- Add a realistic approximately 10 MiB input regression.
- Add a deep-nesting regression.
- Define measurable time and memory limits, or use deterministic bounded-output assertions that detect unbounded growth.
- Document public resource limits and failure behaviour.
## Acceptance criteria
- \`parse_source\` cannot process unbounded input without an explicit documented resource contract.
- Large input at the supported limit has regression coverage.
- Deep conditional nesting has regression coverage.
- Tests detect excessive output or ancestry growth.
- Tests specify and enforce measurable resource limits or deterministic bounded-growth limits.
- CLI diagnostics and existing source-size error behaviour remain stable.
- Documentation states the public parsing and output resource limits.
## Backlinks
- Originating PR: ${pr_url}
- Source request: The GitHub API did not expose a permalink for the request comment at issue creation time.
- Requester: @${requester}
EOF
)"
domain_url="$(gh issue create \
--repo "$repo" \
--title "Separate the parsing domain model from JSON output metadata" \
--body "$domain_body" \
--assignee "$requester")"
performance_url="$(gh issue create \
--repo "$repo" \
--title "Bound public parsing resource use and add scale regressions" \
--body "$performance_body" \
--assignee "$requester")"
printf 'Domain issue: %s\nPerformance issue: %s\n' "$domain_url" "$performance_url"Length of output: 4457
Each issue includes the required changes, affected areas, acceptance criteria, and a backlink to PR You are interacting with an AI system. |
Align the guide filename, title, content, and documentation links with the first unpublished release version.
3 participants
Summary
This branch carries the pre-implementation plan for parsing one explicit GNU
Makefile into deterministic, versioned JSON without evaluating Make syntax or
executing source-selected commands.
ExecPlan: docs/execplans/adr-0001-single-file-gnu-make-parse.md
The plan defines approval-gated milestones for the owned schema, parser port,
source ranges, recovered parses, CLI and stream behaviour, security evidence,
and layered unit, property, snapshot, behavioural and end-to-end testing. It
also imports the local OrthoConfig 0.8.0 user guide used to design the CLI boundary.
No feature implementation is included. Implementation remains blocked until
the plan, including the exact
makefile-lossless = "=0.3.40"exception, isexplicitly approved.
Review walkthrough
ArgMatchesguidance signposted by the plan.Validation
git diff --check: passed.make markdownlint: passed, including Oxford spelling checks.make nixie: passed.make check-fmt: passed.make typecheck: passed with Polonius and warnings denied.make lint: passed, including rustdoc, Clippy and Whitaker.make test: passed, including nextest and doctests.coderabbit review --agent: three completed rounds; all 27 actionable concerns were resolved. The final post-fix retry was rate-limited before analysis and emitted no new findings, so a clean follow-up remains pending after the service window resets.Notes
Wyvern reconnaissance mapped the scaffold and test seams. A full Logisphere
community review revised the architecture and contracts before delivery.
Firecrawl research checked the authoritative
makefile-lossless0.3.40 sourceand API surface.
References