fix(schema): allow assessments with excluded attributes#312
fix(schema): allow assessments with excluded attributes#312kami619 wants to merge 1 commit intoambient-code:mainfrom
Conversation
Relaxes the JSON schema constraints to accept assessments with 10-25 attributes instead of exactly 25. This enables users who run `agentready assess --exclude` to submit valid assessments to the leaderboard. Changes: - `attributes_total`: changed from `const: 25` to `minimum: 10, maximum: 25` - `findings`: changed from `minItems: 25, maxItems: 25` to `minItems: 10, maxItems: 25` Added regression tests to verify: - Assessments with 10 attributes validate (minimum boundary) - Assessments with 22 attributes validate (PR ambient-code#301 case) - Assessments with fewer than 10 attributes are rejected Fixes ambient-code#309
📈 Test Coverage Report
Coverage calculated from unit tests only |
AgentReady Code Review — PR #312Reviewer: Claude Code (AgentReady Review Agent) SummaryThis PR addresses issue #309: users running This is a well-motivated, targeted fix with good test coverage. Strengths
IssuesMedium1. The schema currently allows 2. A report with Minor3. Schema version not bumped The schema title is still 4. Minimum of 10 is undocumented The description field added to 5. Fixture assumption in regression tests The tests mutate Security / Data IntegrityNo security concerns. Schema relaxation is unidirectional (more permissive). The risk of accepting malformed data is low since assessments are user-generated for their own repos. AgentReady Attribute Compliance
RecommendationApprove with suggestions: The core fix is correct and the tests are solid. The schema version bump and a comment on the 10-attribute floor are low-effort improvements worth doing before merge. The cross-field invariant gaps are pre-existing schema limitations not introduced by this PR and are acceptable to defer. |
AgentReady Code ReviewPR: fix(schema): allow assessments with excluded attributes SummaryThis PR fixes a real user-facing issue (#309): assessments generated with Overall: Correct fix for a genuine bug. A few schema consistency concerns to address before moving out of DRAFT. AgentReady Attribute Compliance
Issues🔴 CriticalNone. 🟡 Important1. Schema change without version bump The schema file has 2. No cross-field validation between The schema now allows a submission with if data.get("attributes_total") \!= len(data.get("findings", [])):
errors.append("attributes_total must equal number of findings")3. Minimum of 10 is undocumented The minimum was chosen to "ensure meaningful assessments" but this rationale isn't captured anywhere. Why 10 and not 8 or 15? The 🟢 Suggestions4. Test fixture assumption In valid_report_data["findings"] = valid_report_data["findings"][:22]This silently produces fewer than 22 findings if the fixture has fewer items. Add a guard: assert len(valid_report_data["findings"]) >= 22, "Fixture must have >= 22 findings for this test"
valid_report_data["findings"] = valid_report_data["findings"][:22]5. DRAFT status Once the issues above are addressed, promote to "Ready for Review" so it can be merged before more Strengths
VerdictApprove with suggestions — the core fix is correct and the tests are good. The schema version bump (item 1) and cross-field validation (item 2) are worth addressing; the rest are suggestions. This is close to merge-ready once out of DRAFT. |
AgentReady Code Review — PR #312Reviewed by: Claude Code (AgentReady Review Agent) SummaryThis PR relaxes the assessment schema's AgentReady Attribute Impact
Code QualityStrengths:
Issues: 1. The minimum of 10 is undocumented and arbitrary (medium) The PR description says 'at least 10 attributes to ensure meaningful assessments' but there is no rationale in the schema, tests, or any spec file for why 10 is the right floor rather than, say, 15 or 5. This is a business logic decision that should be explicitly justified in the PR description with reasoning, possibly driven by a minimum coverage per tier. 2. Leaderboard fairness — score inflation risk (medium) This is the most important concern for the long-term health of the leaderboard. When a user excludes attributes, they are more likely to exclude ones their repo fails. A repo that excludes 15 attributes and passes 10 easy ones could achieve a higher certification level than a repo honestly assessed on all 25. Suggested mitigations:
This PR does not have to solve all of this, but a follow-up GitHub issue should track the behavior as intentional. 3. Cross-field consistency not enforced (low) The schema allows inconsistency between fields. Because Test QualityThe three regression tests are well-constructed and cover:
Missing test: What happens when SecurityNo security concerns. This is a pure JSON Schema change with no code execution path changes. Best Practices
Suggested Follow-up IssueCreate a GitHub issue to track: 'Leaderboard fairness for partial assessments (--exclude)' — document whether partial assessments should be scored, ranked, or labeled differently from full 25-attribute assessments. VerdictApprove with follow-up — The schema fix is correct and the tests are solid. The core change is safe and backwards-compatible. The leaderboard fairness concern is real but does not need to block this merge — it should be tracked as a follow-up. Add a brief comment in the PR or linked issue explaining the rationale for the minimum of 10. |
|
@dbasunag WDYT ? |
AgentReady Code Review — PR #312
SummaryThis is a clean, minimal fix that directly addresses the schema rejection of valid IssuesCriticalNo consistency constraint between After this change, the schema accepts Suggestion: add a cross-field validation in MediumMinimum of 10 is arbitrary and undocumented The PR says "ensure meaningful assessments" but does not explain why 10 is the threshold. With 25 total attributes, excluding 15 (60%) seems like a very permissive floor. Document the rationale in the schema's
The Conflicts with PR #315 Both PRs solve issue #309 with different approaches:
They cannot both be merged as-is. PR #315's jq command attempts to re-set MinorPR checklist has multiple unchecked items "My code follows the project's code style", "I have performed a self-review", "I have commented my code", and "I have made corresponding changes to the documentation" are unchecked. Test fixture fragility
assert len(valid_report_data["findings"]) >= 25, "Fixture needs at least 25 findings for this test"What's Good
Attribute Impact
Verdict: Needs revision. The critical gap is the lack of consistency enforcement between |
Description
Relaxes the JSON schema constraints to accept assessments with 10-25 attributes instead of exactly 25. This enables users who run
agentready assess --excludeto submit valid assessments to the leaderboard.Type of Change
Related Issues
Fixes #309
Changes Made
attributes_total: changed fromconst: 25tominimum: 10, maximum: 25findings: changed fromminItems: 25, maxItems: 25tominItems: 10, maxItems: 25Added regression tests to verify:
Testing
pytest)Checklist