Repository navigation
feat: cy-capture-decisions — skill-only extension for durable decision capture - #237
Merged
Merged
Conversation
Add a non-runtime Go package `decisionlog` that parses and validates the decision-record (AD-NNN.md) and index (DECISIONS.md) file-format contract authored in task_01's references. It gives `make verify` a deterministic gate over the log contract and ships no runtime code (test-only, ADR-004): wired to no hook and absent from extension.toml [resources]. Covers assigned unit cases UT-001..UT-008: frontmatter schema (required fields, status enum, evidence-required-for-proven, Reconciliation section), six-field index grammar, active-proven-only membership, empty-state, bidirectional supersession + chain resolution, and broken body-reference detection. Table-driven tests with t.Parallel + testdata golden fixtures. Co-Authored-By: Claude Opus 4.8 <[email protected]>
…sion Adds the skill-only extension.toml manifest (skills.ship + resources.skills; no [subprocess], [[hooks]], agents, or the test-only decisionlog per ADR-004) and the extension-root README documenting install (ext install/enable/setup), usage as the final pipeline step after /cy-final-verify, the produced artifacts, the durability gitignore step (ADR-002), and the @import consumption wiring for CLAUDE.md/AGENTS.md (ADR-001). The README's durability block ships a corrected three-line gitignore negation: the two literal lines named by the spec (!.compozy/DECISIONS.md and !.compozy/decisions/**) are insufficient because git cannot re-include a file whose parent directory is excluded, so a !.compozy/decisions/ directory line is added before the /** negation. Verified against real git. Tests (packaging package, run in make verify): - manifest is skill-only, parsed/validated via the real extension loader - UT-009/010/011: README documents gitignore negations, @import wiring (CLAUDE.md + AGENTS.md), and the after-/cy-final-verify flow position - IT-002: importing the index surfaces its text; decisions/ bodies are not pulled - IT-003: negations flip a .compozy/**-ignored log to tracked (real git) - IT-004: real InstallExtensionSkillPacks lands SKILL.md idempotently E2E-011/012 (consumption journeys) are documented in the eval runbook. Co-Authored-By: Claude Opus 4.8 <[email protected]>
The Decision-record schema section read as though CI validates a user's produced .compozy/DECISIONS.md. It does not: the `decisionlog` package is a test-only asset (ADR-004) with zero exported entry points, so `make verify` only checks the format grammar and its hand-authored fixtures — a malformed real log would not fail CI. Rescope the sentence to what the validator actually guards (the documented grammar and its examples) and state explicitly that it does not run over a project's produced log. Resolves review round 001 issue_002. Co-Authored-By: Claude Opus 4.8 <[email protected]>
The decisionlog CI gate accepted two contract-violating logs: - a record whose frontmatter id differs from its filename stem - a record whose status and superseded_by disagree (a dangling superseded head, or a supersession link on a non-superseded record) when the record is not referenced by the index Add sentinels errIDFilenameMismatch and errStatusLinkMismatch, a filename-identity assertion in loadRecords, and a status/link biconditional check at the top of checkRecordLinks so both run for every record. Cover each at its lowest detecting layer. Resolves reviews-001/issue_001. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Close three index-validation gaps flagged in reviews-002/issue_001, the same class of cross-field-consistency gap round-1 closed for filename/id, now moved up to the DECISIONS.md index line to AD-NNN.md body relationship: - checkIndexRefs now asserts an index line's denormalized title/source_slug match the body it copies (errIndexBodyMismatch), not just that the body is active-proven; a drifted line would import a title/provenance that contradicts the record it points at. - validateIndex rejects duplicate index ids (errDuplicateID), enforcing the index-format "regenerate whole, do not append duplicates" rule. - validateLog adds checkIndexMembership: the reverse half of the membership biconditional (include iff proven and not superseded); every active-proven body must be indexed (errMissingIndexLine), so a proven decision cannot be silently dropped from the loaded surface. Add an index-duplicate-row.md fixture and three subtests (duplicate id row, title/source_slug drift, active-proven body absent from index). Verification: decisionlog fmt/lint clean, go test -race passes (45 tests, 2 packages), extensions build OK. make verify has 4 unrelated pre-existing failures in internal/cli and internal/core/workspace (workspace-config default drift) that reproduce on a clean HEAD worktree; decisionlog is imported by nothing, so this change cannot affect them. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Round-3 issue_002: the evals runbook's Pass criteria said the produced files' format is "additionally guaranteed by the task_02 decisionlog validator, which parses these same fixture-shaped logs in make verify", which reads as a claim that eval-produced logs are format-checked by CI. They are not — the validator parses only hand-authored decisionlog/testdata fixtures. Rescope the sentence to mirror the round-1 issue_002 contract: the validator guards the format grammar via fixtures of this same shape, not the eval-produced logs, whose format is asserted structurally by running the suite. Docs-only; no test pins evals/README.md wording. Co-Authored-By: Claude Opus 4.8 <[email protected]>
validateIndex enforced proven-only membership and duplicate rejection but never
checked the index-format Rules bullet "Sort lines by AD-NNN ascending so diffs stay
stable across captures", so a regenerated-but-unsorted DECISIONS.md passed the gate
and churned cross-capture diffs.
Add errIndexUnsorted and reject a descending step, comparing on the numeric value
of the \d{3,} id suffix (adIDLess) rather than lexically, so AD-1000 sorts after
AD-999 at any id width. Add an unsorted testdata fixture plus subtests for descending
3-digit ids and multi-width numeric ordering.
Resolves reviews-003/issue_001.
Co-Authored-By: Claude Opus 4.8 <[email protected]>
…suite Adds the pieces the initial packaging commit (5611be0) left out, so the committed extension is actually functional rather than validator-only: - skills/cy-capture-decisions/SKILL.md and its three references (decision-record-template, index-format, reconciliation-guide) — the capture skill's procedural core plus the AD-NNN.md / DECISIONS.md format contracts that the decisionlog validator enforces. - evals/ behavioral eval runbook and six curated fixture workflows (feat-orders / payments / search / auth / noop / telemetry) mapping every assigned E2E/IT case to a fixture and its expected log properties. - README quick-start navigation section. Incorporates review rounds 1-3: filename/id and status/link consistency, index/body membership, the evals README scope fix, and ascending AD-NNN index ordering (numeric, not lexical) — each with fixtures and subtests. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Step 7's id allocation used `ls AD-*.md | wc -l` + 1, which only yields the correct next id when the AD-NNN numbering is contiguous. A gap (from a manual repair, import, or interrupted prior capture, US-006.EC-2) made the count reselect an existing id — overwriting a real decision body and violating the "unique across the whole log. Never reused" contract. Derive the next id from the maximum existing suffix instead, preserving the write-then-recompute ordering from round-1 issue_003. Resolves reviews-004 issue_002. Co-Authored-By: Claude Opus 4.8 <[email protected]>
The README Quick start and Install sections showed a bare `compozy ext install`, but the CLI declares Args: cobra.ExactArgs(1) (internal/cli/extension/install.go), so a zero-argument invocation fails with "accepts 1 arg(s), received 0" before any install logic runs — a hard onboarding failure for a skill-only extension whose README is the whole product surface. Replace both call sites with the canonical first-party bundle form used by the sibling cy-idea-factory extension (compozy/compozy --remote github --ref <tag> --subdir extensions/cy-capture-decisions), and add a packaging regression test that fails on any bare `compozy ext install` and asserts the canonical source form is present. Resolves reviews-004/issue_001. Co-Authored-By: Claude Opus 4.8 <[email protected]>
…capture-decisions The decision-record-template reference still prescribed the buggy `(count of AD-*.md) + 1` id derivation that commit 3322132 removed from SKILL.md step 7. An agent reading the template as the authoritative schema could reuse an id across a numbering gap and overwrite an existing decision body. Point the template at SKILL.md step 7 as the single source of truth (max-suffix derivation) so the two docs can no longer drift. Also add a worked degraded-mode example (diff unscopable -> candidate, "unverified against code", excluded from index), the highest-nuance branch that previously had only prose. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Contributor
|
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:
WalkthroughAdds a decision-log validator, packaging and README contract tests, a model-backed evaluation harness with E2E/IT cases, a Makefile evaluation command, and isolated configuration test execution. ChangesDecision log validation
Evaluation infrastructure
Packaging contracts
Test isolation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant EvalCommand
participant EvaluationHarness
participant CompozyRuntime
participant Workspace
EvalCommand->>EvaluationHarness: configure and run selected cases
EvaluationHarness->>CompozyRuntime: install extension and start daemon
EvaluationHarness->>Workspace: create fixture repository and skill
EvaluationHarness->>CompozyRuntime: run model capture
EvaluationHarness->>Workspace: archive decision artifacts and diffs
EvaluationHarness->>EvalCommand: report trial results and summaries
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
antoniofulg
marked this pull request as ready for review
July 13, 2026 02:30
|
| Filename | Overview |
|---|---|
| extensions/cy-capture-decisions/README.md | Documents install, capture usage, gitignore durability, and agent-memory wiring; the gitignore verification command still describes the wrong git check-ignore behavior. |
| extensions/cy-capture-decisions/decisionlog/decisionlog.go | Adds the decision-log validator for frontmatter schema, index/body consistency, supersession links, and numeric decision ordering. |
| extensions/cy-capture-decisions/skills/cy-capture-decisions/SKILL.md | Defines the capture workflow, evidence gates, promotion rules, and reserved delimiter rules for generated decision logs. |
| extensions/cy-capture-decisions/packaging/conventions_test.go | Adds packaging convention tests, including real gitignore checks for ignored and re-included decision-log paths. |
Reviews (10): Last reviewed commit: "Merge branch 'main' into feat/cy-capture..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?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: b61bc0c2ac
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
extensions/cy-capture-decisions/decisionlog/decisionlog_test.go (1)
24-350: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSubtest names don't follow the required
Should...convention.Every
t.Run(...)subtest here uses a descriptive name (e.g."full proven record parses every frontmatter field") rather than the mandatedShould...form (e.g."Should parse every frontmatter field of a full proven record"). Please rename subtests accordingly.As per path instructions: "MUST use t.Run("Should...") pattern for ALL test cases".
🤖 Prompt for 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. In `@extensions/cy-capture-decisions/decisionlog/decisionlog_test.go` around lines 24 - 350, Rename every subtest name passed to t.Run across TestParseDecisionRecord, TestParseDecisionRecordErrors, TestParseIndexLine, TestValidateIndex, TestValidateSupersession, and TestValidateLog so each begins with the exact “Should...” convention. Preserve each test’s existing behavior and assertions while converting descriptive names, including dynamically constructed grammar and membership case names, into Should-prefixed names.Source: Path instructions
🤖 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.
Nitpick comments:
In `@extensions/cy-capture-decisions/decisionlog/decisionlog_test.go`:
- Around line 24-350: Rename every subtest name passed to t.Run across
TestParseDecisionRecord, TestParseDecisionRecordErrors, TestParseIndexLine,
TestValidateIndex, TestValidateSupersession, and TestValidateLog so each begins
with the exact “Should...” convention. Preserve each test’s existing behavior
and assertions while converting descriptive names, including dynamically
constructed grammar and membership case names, into Should-prefixed names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 88b14c9b-4195-4314-8506-9c88580c2e93
⛔ Files ignored due to path filters (56)
extensions/cy-capture-decisions/README.mdis excluded by!**/*.mdextensions/cy-capture-decisions/decisionlog/testdata/index-candidate-row.mdis excluded by!**/*.mdextensions/cy-capture-decisions/decisionlog/testdata/index-duplicate-row.mdis excluded by!**/*.mdextensions/cy-capture-decisions/decisionlog/testdata/index-empty.mdis excluded by!**/*.mdextensions/cy-capture-decisions/decisionlog/testdata/index-superseded-row.mdis excluded by!**/*.mdextensions/cy-capture-decisions/decisionlog/testdata/index-unsorted.mdis excluded by!**/*.mdextensions/cy-capture-decisions/decisionlog/testdata/index-valid.mdis excluded by!**/*.mdextensions/cy-capture-decisions/decisionlog/testdata/record-bad-status.mdis excluded by!**/*.mdextensions/cy-capture-decisions/decisionlog/testdata/record-candidate-no-evidence.mdis excluded by!**/*.mdextensions/cy-capture-decisions/decisionlog/testdata/record-missing-field.mdis excluded by!**/*.mdextensions/cy-capture-decisions/decisionlog/testdata/record-no-reconciliation.mdis excluded by!**/*.mdextensions/cy-capture-decisions/decisionlog/testdata/record-proven-no-evidence.mdis excluded by!**/*.mdextensions/cy-capture-decisions/decisionlog/testdata/record-valid.mdis excluded by!**/*.mdextensions/cy-capture-decisions/decisionlog/testdata/valid-log/DECISIONS.mdis excluded by!**/*.mdextensions/cy-capture-decisions/decisionlog/testdata/valid-log/decisions/AD-001.mdis excluded by!**/*.mdextensions/cy-capture-decisions/decisionlog/testdata/valid-log/decisions/AD-002.mdis excluded by!**/*.mdextensions/cy-capture-decisions/decisionlog/testdata/valid-log/decisions/AD-003.mdis excluded by!**/*.mdextensions/cy-capture-decisions/decisionlog/testdata/valid-log/decisions/AD-004.mdis excluded by!**/*.mdextensions/cy-capture-decisions/evals/README.mdis excluded by!**/*.mdextensions/cy-capture-decisions/evals/fixtures/feat-auth/diff.patchis excluded by!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-auth/expected.mdis excluded by!**/*.md,!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-auth/seed-log/DECISIONS.mdis excluded by!**/*.md,!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-auth/seed-log/decisions/AD-001.mdis excluded by!**/*.md,!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-auth/workflow/adrs/adr-001.mdis excluded by!**/*.md,!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-auth/workflow/task_01.mdis excluded by!**/*.md,!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-noop/expected.mdis excluded by!**/*.md,!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-noop/workflow/adrs/adr-001.mdis excluded by!**/*.md,!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-noop/workflow/adrs/adr-002.mdis excluded by!**/*.md,!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-orders/diff.patchis excluded by!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-orders/expected.mdis excluded by!**/*.md,!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-orders/workflow/adrs/adr-001.mdis excluded by!**/*.md,!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-orders/workflow/adrs/adr-002.mdis excluded by!**/*.md,!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-orders/workflow/adrs/adr-003.mdis excluded by!**/*.md,!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-orders/workflow/memory/MEMORY.mdis excluded by!**/*.md,!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-orders/workflow/reviews-001/issue_001.mdis excluded by!**/*.md,!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-orders/workflow/task_01.mdis excluded by!**/*.md,!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-payments/diff.patchis excluded by!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-payments/expected.mdis excluded by!**/*.md,!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-payments/workflow/adrs/adr-001.mdis excluded by!**/*.md,!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-payments/workflow/adrs/adr-002.mdis excluded by!**/*.md,!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-payments/workflow/task_01.mdis excluded by!**/*.md,!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-search/diff-phase2.patchis excluded by!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-search/diff.patchis excluded by!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-search/expected.mdis excluded by!**/*.md,!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-search/workflow/adrs/adr-001.mdis excluded by!**/*.md,!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-search/workflow/adrs/adr-002.mdis excluded by!**/*.md,!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-search/workflow/memory/MEMORY.mdis excluded by!**/*.md,!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-telemetry/expected.mdis excluded by!**/*.md,!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-telemetry/workflow/adrs/adr-001.mdis excluded by!**/*.md,!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-telemetry/workflow/memory/MEMORY.mdis excluded by!**/*.md,!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-telemetry/workflow/reviews-001/issue_001.mdis excluded by!**/*.md,!**/fixtures/**extensions/cy-capture-decisions/extension.tomlis excluded by!**/*.tomlextensions/cy-capture-decisions/skills/cy-capture-decisions/SKILL.mdis excluded by!**/*.mdextensions/cy-capture-decisions/skills/cy-capture-decisions/references/decision-record-template.mdis excluded by!**/*.mdextensions/cy-capture-decisions/skills/cy-capture-decisions/references/index-format.mdis excluded by!**/*.mdextensions/cy-capture-decisions/skills/cy-capture-decisions/references/reconciliation-guide.mdis excluded by!**/*.md
📒 Files selected for processing (7)
extensions/cy-capture-decisions/decisionlog/decisionlog.goextensions/cy-capture-decisions/decisionlog/decisionlog_test.goextensions/cy-capture-decisions/packaging/conventions_test.goextensions/cy-capture-decisions/packaging/doc.goextensions/cy-capture-decisions/packaging/helpers_test.goextensions/cy-capture-decisions/packaging/manifest_test.goextensions/cy-capture-decisions/packaging/readme_test.go
…e docs Address two Greptile findings on the cy-capture-decisions PR: - P1 (index-format): the DECISIONS.md index is split on `|` into exactly six fields, but the Title and rationale were documented as free text. A literal `|` in a title (e.g. "JWT | opaque tokens") would produce an index line the decisionlog validator rejects, and — since checkIndexRefs enforces line.Title == body.title exactly — the title itself must stay pipe-free, not just its index rendering. Reserve `|` as the delimiter and render any pipe as `/` in the title and rationale, documented at each natural site (field definition, index grammar, and SKILL critical rules). - P2 (README): the gitignore-durability step claimed `git check-ignore <path>` reports re-included paths "as tracked", but the plain command prints nothing and exits 1 for a non-ignored path. Switch the verification to `git check-ignore -v`, which prints the matching negation rule and exits 0, and explain the plain command's silent exit-1 signal. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Address a Codex review finding: checkIndexRefs compared only the index line's title and source_slug to the AD-NNN body, so a stale or wrong [tags] list in DECISIONS.md passed validation even though index-format.md defines [tags] as the body's tags. Because the index is the imported scoped-loading surface, a wrong tag would silently mislead future sessions while make verify stayed green. Extend the cross-check to require the line's tags to equal the body's tags (slices.Equal, order-sensitive to match how the index renders them), and add a TestValidateLog subtest that a tag drift is rejected with errIndexBodyMismatch. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Add a README Acknowledgements section crediting Felipe Rodrigues's tlc-spec-driven skill (Tech Leads Club, CC-BY-4.0) for the AD-NNN decision-log pattern with supersession and a project-durability relevance gate. cy-capture-decisions is an independent reworking of that idea and reuses the concept, not the source text. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
TestApplyWorkspaceDefaultsFetchReviewsNitpicks and TestLoadConfigTaskTypes read the ambient HOME config instead of an isolated temp home, so a local ~/.compozy/config.toml (fetch-reviews nitpicks, task types) leaked into the "config absent" cases and failed them locally — while CI stayed green because it has no such file. Add the existing isolateCLIConfigHome / isolateWorkspaceConfigHome helpers and drop t.Parallel (incompatible with their t.Setenv), matching the isolation already applied to the sibling tests in these files. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Contributor
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 `@internal/core/workspace/config_test.go`:
- Around line 1213-1216: Update the subtest naming in the test loop around
isolateWorkspaceConfigHome by changing t.Run to prefix each case name with
"Should ". Preserve the existing tt.name value and test body unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 39b053ee-7c71-48a3-82a7-7118f7347b12
⛔ Files ignored due to path filters (1)
extensions/cy-capture-decisions/README.mdis excluded by!**/*.md
📒 Files selected for processing (2)
internal/cli/workspace_config_test.gointernal/core/workspace/config_test.go
pedronauck
requested changes
Jul 15, 2026
pedronauck
left a comment
pedronauck
left a comment
Member
There was a problem hiding this comment.
The deterministic format/packaging coverage is useful, but it does not validate the product this PR ships: the LLM-driven reconciliation workflow. evals/README.md defines 21 E2E/IT cases and explicitly requires each eval to run 3×, yet the PR contains only a manual runbook/fixtures—there is no executable harness, CI job, or attached run evidence for the produced logs. make verify can stay green even if the skill promotes the wrong ADRs, breaks idempotency, overwrites a manual correction, or mishandles degraded mode.
Please provide an executable opt-in model-backed harness (or run and attach reproducible structural results for the full assigned matrix, 3× as specified) that installs the shipped skill and asserts the generated artifacts. At minimum it must cover promotion/relevance, candidate→proven, idempotent rerun, supersession, degraded diff, malformed input, unwritable/partial-write protection, and consumption. Also resolve the contract contradiction around concurrency: E2E-021 says two captures must not corrupt the log, while the skill Error Handling says concurrent runs are unsupported. Choose one guarantee, align the skill/runbook, and prove it. Then get the repository verify check green on the final SHA.
Contributor
Author
|
@pedronauck Thanks, I added an executable, opt-in model-backed harness in 88e280a. It installs the extension and its shipped skill into an isolated Compozy environment, runs the workflow in scratch repositories, validates the generated decision log, and applies structural assertions for each case. It checks IDs, provenance, status, evidence, supersession, index membership, idempotency, and file safety without depending on exact model wording. The matrix now has 24 cases and runs each case three times by default, for 72 trials. It covers the promotion and relevance rules, candidate-to-proven transitions, idempotent reruns, supersession, degraded diff handling, malformed input, unwritable and partial-write protection, VCS reviewability, and decision consumption. Each trial keeps its raw ACP events, stderr, generated artifacts, Git diff, and result summary. COMPOZY_EVAL_MODEL=gpt-5.6-luna \
COMPOZY_EVAL_REASONING_EFFORT=medium \
make eval-cy-capture-decisionsFor the concurrency contradiction, I chose a serial single-writer contract. Concurrent captures in the same workspace are unsupported. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (1)
Makefile (1)
164-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the recipe within checkmake's body limit.
The new target has eight recipe lines, exceeding the configured five-line limit. Collapse the model guard into one line.
Proposed fix
- `@if` [ -z "$(COMPOZY_EVAL_MODEL)" ]; then \ - echo "$(RED)Error: COMPOZY_EVAL_MODEL is required$(NC)"; \ - exit 1; \ - fi + `@test` -n "$(COMPOZY_EVAL_MODEL)" || { echo "$(RED)Error: COMPOZY_EVAL_MODEL is required$(NC)"; exit 1; }🤖 Prompt for 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. In `@Makefile` around lines 164 - 172, Collapse the COMPOZY_EVAL_MODEL validation in the eval-cy-capture-decisions recipe into a single shell line while preserving its error message and exit status, keeping the total recipe within checkmake’s five-line limit.Source: Linters/SAST tools
🤖 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 `@extensions/cy-capture-decisions/evals/cases.go`:
- Around line 732-745: Update the record validation around loadLog and
findByProvenance to require exactly two snapshot records, rejecting duplicates
or extras. Also verify that the original AD-001 record remains present, while
preserving the existing check that the newly found weak-match record does not
have ID AD-001.
- Around line 645-668: Update runE2E018 so each bad-slug prompt creates its own
fresh workspace via newWorkspace, runs the model, and checks hasDecisionLog
immediately within that iteration. Preserve the existing error and “slug”
response checks, and fail that iteration if its workspace contains a decision
log.
- Around line 970-992: The assertSupersession helper currently verifies only
that oldID is absent from the index. Extend its final validation to also
requireIndexed for newID with the indexed expectation set to true, while
preserving the existing superseded-record checks and failure behavior.
- Around line 784-821: Update runIT005 to enforce a genuinely unwritable sandbox
rather than relying only on setTreePermissions/os.Chmod. Skip the case on
unsupported platforms or execute t.capture against an unprivileged read-only
mount, while preserving restoration, tree-integrity validation, and
write-failure evidence checks.
In `@extensions/cy-capture-decisions/evals/cmd/cy-capture-decisions-eval/main.go`:
- Around line 37-40: Update the main harness flow around evals.Run to create a
signal-aware context with signal.NotifyContext for SIGINT and SIGTERM, defer its
cancellation, and pass that context instead of context.Background(). Preserve
the existing runErr reporting and process exit behavior.
In `@extensions/cy-capture-decisions/evals/fixtures_test.go`:
- Around line 22-26: Update the fixture discovery setup around filepath.Glob to
fail the test when no fixture directories are found. After confirming the
existing Glob error handling, validate that fixtures is non-empty before the
loop and call t.Fatalf with a clear message when discovery returns zero results.
In `@extensions/cy-capture-decisions/evals/harness_test.go`:
- Around line 9-66: Wrap each scenario in harness_test.go, including
TestAllCasesDefinesCompleteAssignedMatrix,
TestSelectCasesNormalizesIDsAndPreservesMatrixOrder, and
TestDefaultConfigRequiresOptInModelAndThreeRuns, in descriptive t.Run subtests
whose names start with “Should”. In fixtures_test.go, prefix existing fixture
subtest names with “Should” and wrap the FitnessHub scenario in a corresponding
“Should...” t.Run block; ensure every test case in both affected files follows
this convention.
In `@extensions/cy-capture-decisions/evals/harness.go`:
- Around line 345-355: The initializeRepository method must neutralize inherited
Git behavior before creating the base commit. Extend its local git config setup
to disable commit signing and set core.hooksPath to an empty hooks directory,
while preserving the existing user.name and user.email configuration and commit
flow.
- Around line 151-153: Update the cleanup defer around stopDaemon to pass the
run context, wrapping it with context.WithoutCancel before applying the cleanup
timeout. Modify stopDaemon and its callers as needed so daemon cleanup preserves
request values while remaining independent of parent cancellation.
- Around line 419-425: Update the artifact creation in the harness around
stdoutFile and stderrFile to use os.OpenFile with creation and write flags and
permissions 0o600, preserving the existing stdoutPath/stderrPath names and error
handling for both raw model capture files.
- Around line 447-450: Update the command construction in the harness execution
path to invoke t.harness.config.CompozyBinary directly instead of using "env" as
the executable. Keep commandArgs, cmd.Dir, and cmd.Env behavior unchanged so the
configured environment is still applied and Windows execution works.
- Around line 395-401: Update installSkill to derive the setup target from
t.harness.config.IDE using agent.SetupAgentName, and pass that value to the
setup command instead of the hardcoded "codex" argument. Preserve the existing
skill name, copy, confirmation, and error-handling behavior.
In `@extensions/cy-capture-decisions/evals/log.go`:
- Around line 20-50: Update loadLog to wrap validation and file-read failures
with explicit workspace, index, or decision filename context using fmt.Errorf
with %w. Add context around decisionlog.Validate, os.ReadFile for each decision
body, and the final DECISIONS.md read while preserving existing error
propagation.
- Around line 64-72: The record lookup in findByProvenance can return an
arbitrary record when multiple entries share the same SourceSlug and SourceADR.
Update validateLog to reject duplicate provenance pairs, or modify
findByProvenance to detect multiple matches and return an error instead of
selecting one; preserve the existing not-found behavior.
---
Nitpick comments:
In `@Makefile`:
- Around line 164-172: Collapse the COMPOZY_EVAL_MODEL validation in the
eval-cy-capture-decisions recipe into a single shell line while preserving its
error message and exit status, keeping the total recipe within checkmake’s
five-line limit.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b7016284-d9e0-4592-a004-ec5eed64d138
⛔ Files ignored due to path filters (11)
extensions/cy-capture-decisions/README.mdis excluded by!**/*.mdextensions/cy-capture-decisions/evals/README.mdis excluded by!**/*.mdextensions/cy-capture-decisions/evals/examples/fitnesshub-web/.compozy/DECISIONS.mdis excluded by!**/*.mdextensions/cy-capture-decisions/evals/examples/fitnesshub-web/.compozy/decisions/AD-011.mdis excluded by!**/*.mdextensions/cy-capture-decisions/evals/examples/fitnesshub-web/.compozy/decisions/AD-014.mdis excluded by!**/*.mdextensions/cy-capture-decisions/evals/examples/fitnesshub-web/.compozy/decisions/AD-016.mdis excluded by!**/*.mdextensions/cy-capture-decisions/evals/fixtures/feat-auth/diff.patchis excluded by!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-orders/diff.patchis excluded by!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-payments/diff.patchis excluded by!**/fixtures/**extensions/cy-capture-decisions/evals/fixtures/feat-search/diff-phase2.patchis excluded by!**/fixtures/**extensions/cy-capture-decisions/skills/cy-capture-decisions/SKILL.mdis excluded by!**/*.md
📒 Files selected for processing (9)
Makefileextensions/cy-capture-decisions/decisionlog/decisionlog.goextensions/cy-capture-decisions/evals/cases.goextensions/cy-capture-decisions/evals/cmd/cy-capture-decisions-eval/main.goextensions/cy-capture-decisions/evals/fixtures_test.goextensions/cy-capture-decisions/evals/harness.goextensions/cy-capture-decisions/evals/harness_test.goextensions/cy-capture-decisions/evals/log.gointernal/core/workspace/config_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/core/workspace/config_test.go
- extensions/cy-capture-decisions/decisionlog/decisionlog.go
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
extensions/cy-capture-decisions/evals/harness.go (1)
639-643: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEscape pipe characters in Markdown table output.
If the
SkipReasonorErrorstring contains a pipe (|) character, it will break the Markdown table formatting. Consider replacing pipes to ensure the summary table always renders correctly.♻️ Proposed refactor
if result.Skipped { - status = "SKIP: " + strings.ReplaceAll(result.SkipReason, "\n", " ") + status = "SKIP: " + strings.ReplaceAll(strings.ReplaceAll(result.SkipReason, "\n", " "), "|", " ") } else if !result.Passed { - status = "FAIL: " + strings.ReplaceAll(result.Error, "\n", " ") + status = "FAIL: " + strings.ReplaceAll(strings.ReplaceAll(result.Error, "\n", " "), "|", " ") }🤖 Prompt for 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. In `@extensions/cy-capture-decisions/evals/harness.go` around lines 639 - 643, Update the status formatting in the result-summary logic to escape pipe characters in both result.SkipReason and result.Error, while preserving the existing newline replacement and SKIP/FAIL prefixes so Markdown table output remains valid.
🤖 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 `@extensions/cy-capture-decisions/evals/harness_test.go`:
- Around line 103-105: Update the duplicate provenance assertion around
findByProvenance to avoid matching the ordered substring “AD-001, AD-002”;
verify the error is non-nil and independently contains both “AD-001” and
“AD-002”, regardless of their order.
---
Nitpick comments:
In `@extensions/cy-capture-decisions/evals/harness.go`:
- Around line 639-643: Update the status formatting in the result-summary logic
to escape pipe characters in both result.SkipReason and result.Error, while
preserving the existing newline replacement and SKIP/FAIL prefixes so Markdown
table output remains valid.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cb2810df-c9cd-4af7-a01a-6ee7794af2e2
⛔ Files ignored due to path filters (1)
extensions/cy-capture-decisions/evals/README.mdis excluded by!**/*.md
📒 Files selected for processing (8)
extensions/cy-capture-decisions/evals/cases.goextensions/cy-capture-decisions/evals/cmd/cy-capture-decisions-eval/main.goextensions/cy-capture-decisions/evals/cmd/cy-capture-decisions-eval/signal_other.goextensions/cy-capture-decisions/evals/cmd/cy-capture-decisions-eval/signal_unix.goextensions/cy-capture-decisions/evals/fixtures_test.goextensions/cy-capture-decisions/evals/harness.goextensions/cy-capture-decisions/evals/harness_test.goextensions/cy-capture-decisions/evals/log.go
🚧 Files skipped from review as they are similar to previous changes (4)
- extensions/cy-capture-decisions/evals/cmd/cy-capture-decisions-eval/main.go
- extensions/cy-capture-decisions/evals/log.go
- extensions/cy-capture-decisions/evals/fixtures_test.go
- extensions/cy-capture-decisions/evals/cases.go
pedronauck
requested changes
Jul 16, 2026
pedronauck
left a comment
pedronauck
left a comment
Member
There was a problem hiding this comment.
Re-review completed at head ed5178c973314d5b8e61b04e8bdadef080279b45. The prior executable-harness and concurrency-contract findings are addressed, all 19 review threads are resolved, and the focused race suite passed 73 tests across the extension packages. This head still targets the older base 5a26ff3, and there is no successful current merge-ref make verify. Updating it while the authenticated GitHub PR-files endpoint is returning HTTP 503 would deterministically fail the CI path-filter before verification, as proven by #236 run 29540121584. Please update against current main and run the complete canonical gate once that API recovers. No code or test workaround is requested; approval requires a green exact-tree make verify.
Contributor
Author
|
btw, if you have another name in mind that may fit better for this extension, let me know @pedronauck |
pedronauck
approved these changes
Jul 17, 2026
pedronauck
left a comment
pedronauck
left a comment
Member
There was a problem hiding this comment.
Resolved current-main conflicts directly while preserving both config-home isolation and the mandatory extension-module gate. The PR merge ref tree exactly matches verified candidate dc611ce. Official run 29543373848 passed formatting, golangci-lint with 0 issues, 4,871 Go tests (4 intentional skips), build, extension-module verification, and 5 Playwright E2E tests.
This was referenced Jul 17, 2026
antoniofulg
added a commit
to antoniofulg/compozy
that referenced
this pull request
Jul 23, 2026
cy-capture-decisions (compozy#237) and cy-qa-workflow (compozy#234) are independent skill-only extensions with zero coupling to task groups; they belong in their own PRs. Removing them keeps this PR scoped to Task Groups.
antoniofulg
added a commit
to antoniofulg/compozy
that referenced
this pull request
Jul 24, 2026
Reverts six PRs that rode along on this branch but are independent of the Task Groups feature (verified: no task-group code depends on them): - compozy#234 cy-qa-workflow module + host.tasks.create v2 (extension SDK) - compozy#237 cy-capture-decisions extension - compozy#226 Kiro CLI ACP model handling - compozy#248 sync-test isolation (touched cy-capture-decisions) - compozy#252 release v0.2.15 prep - compozy#253 test-daemon reaping Kept the task-group-relevant infra: compozy#230 (multi-run recovery), compozy#236 (reviews-fix progress), compozy#241 (parallel task status), compozy#250 (task artifacts).
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds the
cy-capture-decisionsskill-only Compozy extension: it captures a finished workflow'sdurable decisions into a two-tier, project-scoped decision log — a terse
.compozy/DECISIONS.mdindexplus rich
.compozy/decisions/AD-NNN.mdbodies — so future features start informed by what past onesdecided and proved.
Run as the final step of a workflow, it reconciles the plan (the workflow's
AcceptedADRs) againstthe settled reality (the code diff, review issues, and task status) and promotes only the decisions that
are both proven (backed by cited evidence) and cross-feature-durable.
Motivation
Compozy's ADRs live under
.compozy/tasks/<slug>/adrs/, scoped to the workflow that produced them — bydesign. That leaves a gap for the next feature:
compozy archive), so their decisions leave the workingset once the feature ships.
remediation — so reading a raw old ADR can mislead.
planning.
Without a durable, reconciled, cross-feature record to consult, a new feature can re-litigate a settled
question or contradict prior architecture. This extension adds that missing layer — with no core
changes.
How it works
Two tiers, both at the workspace root (siblings of
.compozy/tasks/, so they survivecompozy archive):.compozy/DECISIONS.md— a terse index, one line per active,provendecision.@imported onceinto agent memory (
CLAUDE.md/AGENTS.md), so every future session (planning included) loads itautomatically.
.compozy/decisions/AD-NNN.md— a rich body per decision (original ADR sections + a## Reconciliationnote), read on demand when new work touches that area.A relevance gate (cross-feature-durable and non-obvious and future-relevant) keeps feature-local
noise out; when in doubt it does not promote — a missed decision can be captured on a later re-run,
a wrongly-promoted one is permanent noise. Capture is idempotent (a re-run on an unchanged workflow
is a no-op) and degrades safely when the diff can't be scoped (writes
candidate, neverproven).What's in the extension
skills/cy-capture-decisions/— the capture skill (SKILL.md) and its three references: the reconciliation guide (relevance gate, evidence rules, classification, supersession, degraded mode), theAD-NNN.mddecision-record template, and theDECISIONS.mdindex format.decisionlog/— a Go format validator (test-only asset, ADR-004): a deterministicmake verifygate over the log-format contract, exercised against hand-authoredtestdata/fixtures. Enforces frontmatter schema, status enum, evidence-for-proven, six-field index grammar, filename↔id and status↔link consistency, index↔body membership (biconditional), bidirectional supersession + cycle detection, and ascendingAD-NNNordering (numeric, not lexical).packaging/— integration tests for@importconsumption,.gitignoredurability negations (realgit check-ignore), and idempotent skill install via the real setup path.evals/— a behavioral eval runbook + six fixture workflows mapping every assigned E2E/IT case to expected log properties (LLM-behavior suite; not part ofmake verify).Skill-only: no runtime process, no hooks, no core changes. The read-side consumption wiring (
@import,.gitignorenegations) is documented, not auto-applied — the skill never edits your.gitignore,CLAUDE.md, orAGENTS.md.Try it before merging (install straight from the PR branch)
You don't need a release tag — install from the PR branch into your own Compozy project, do the
one-time setup, and dogfood it on a few real features before deciding to merge. Full walkthrough is in
extensions/cy-capture-decisions/README.md; the condensedpath:
1. Install the extension from the PR branch
Nothing touches your project until you run the skill — the install stays disabled until you enable it.
Steps 2 and 3 are one-time wiring you apply by hand; the skill deliberately never edits your
.gitignore,CLAUDE.md, orAGENTS.md.2. Keep the log committed — only if your repo ignores
.compozy/**The log is only useful once it's committed and shared. If your
.gitignoreignores.compozy/**(skeeper-managed repos — and Compozy itself — do), re-include the log with these negations:
The middle line re-includes the directory itself — git won't re-include a file whose parent dir is still
excluded, so all three lines are required. If your repo already commits
.compozy/(or has no.gitignore), skip this: the log is tracked by default.3. Auto-load the index into every session
Add this one line to
CLAUDE.mdand/orAGENTS.md(whichever your project uses), once per file:Only the terse index is imported; the rich
AD-NNN.mdbodies are read on demand, so context cost staysbounded as the log grows. Without this line the index just isn't auto-loaded — consumption degrades to
reading
.compozy/DECISIONS.mdby hand.4. Capture, as the final step of a workflow
Run it after a clean
/cy-final-verifyon 2–3 of your own features:After a couple of features you'll have
.compozy/DECISIONS.mdplus.compozy/decisions/AD-NNN.mdatyour workspace root. That's the real acceptance test: are the promoted decisions the ones actually worth
carrying forward, did the relevance gate filter the feature-local noise, is a re-run on an unchanged
workflow a clean
no changes— and, thanks to step 3, does the index show up in a fresh session'scontext with no manual step?
Optional fast sanity check (~2s, no agent)
If you'd rather start with a zero-setup check, the shipped Go suite validates the decision-log format
grammar and the packaging, and is already part of
make verify:go test ./extensions/cy-capture-decisions/...Review history
Rounds 1–3 (self-review via
/cy-review-round) are all resolved: filename/id + status/link gaps, index/body consistency + reverse membership + duplicate lines, the evals-README scope overclaim, and the ascending-order rule — each closed at root with a dedicated sentinel, fixture, and subtest. Round 4 was clean.Verification status — read before merging
The whole branch is extension-scoped: the diff against
maintouches onlyextensions/cy-capture-decisions/**(verified — nothing outside it changes). Within that scope it isgreen — repo-wide
fmtandlintpass with 0 issues, and both extension packages (decisionlog,packaging) pass.A full-module
make verifycurrently shows two failing tests, both pre-existing onmainandunrelated to this branch:
internal/cli TestApplyWorkspaceDefaultsFetchReviewsNitpicks(workspace_config_test.go:368)internal/core/workspace TestLoadConfigTaskTypes(config_test.go:1238)Both concern workspace-config defaults (
reviews.fetch.nitpicks,tasks.types). Isolation proof: bothfailing test files are byte-identical to
main(git diff main HEAD --on them is empty), andneither failing package imports the extension (it's a leaf) — so nothing in this branch can cause them.
Follow-ups before merge: resolve or baseline the two pre-existing failures; run
deslopon thebranch diff.
🤖 Generated with Claude Code
Summary by CodeRabbit
make eval-cy-capture-decisionstarget (configurable via environment variables).Acknowledgements
Inspired by Felipe Rodrigues's
tlc-spec-drivenskill (Tech Leads Club, CC-BY-4.0) — specifically the durableAD-NNNdecision-log pattern with supersession and a project-durability relevance gate.cy-capture-decisionsis an independent reworking of that idea (post-pipeline reconciliation-and-promotion, evidence gating, a two-tier@importindex, and a Go grammar validator); it reuses the concept, not the source text.