Skip to content

chore: add untrusted-input and constructor-validation standards, widen pr-review spec axis - #4399

Open
jarhodes314 wants to merge 1 commit into
thin-edge:mainfrom
jarhodes314:jarhodes314/pr-review-spec-axis-and-untrusted-input
Open

jarhodes314 wants to merge 1 commit into
thin-edge:mainfrom
jarhodes314:jarhodes314/pr-review-spec-axis-and-untrusted-input

Conversation

@jarhodes314

@jarhodes314 jarhodes314 commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Proposed changes

A manual review of #4381 found three problems that the pr-review skill would not have flagged, because no review axis questions the spec itself or looks outside the diff: a value from a command payload reaching sudo as an option, an unnecessary sudo -n --list probe that the spec asked for, and stale docs and help text that still show the old pattern. A fourth finding (validation in TryFrom<TomlOperationWorkflow> that #[serde(try_from)] bypasses) had no written standard to point to.

 CODING_STANDARDS.md
   Correctness
     Errors
+      Validate in the type's own constructor, so serde and tests can't bypass it
   thin-edge.io specifics
+    Untrusted input: a payload value reaching spawn/sudo/path/shell/URL cannot be read as something else (option, env assignment, other directory, other command, other URL)
 .agents/skills/pr-review/SKILL.md
   Spec prompt
     (a) missing  (b) unrequested  (c) wrong implementation
+    (d) the spec asks for the wrong solution: name the simpler or safer one
+    search the repo outside the diff for remaining uses of a replaced pattern

Both new standards land in sections that the Correctness axis already checks, so the skill's section routing is unchanged.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
  • Documentation Update (if none of the other choices apply)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

No issue. Follow-up from the review of #4381.

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA (in all commits with git commit -s. You can activate automatic signing by running just prepare-dev once)
  • I ran just format as mentioned in CODING_GUIDELINES
  • I used just check as mentioned in CODING_GUIDELINES
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

The stale-pattern search sits in the Spec prompt, so it only runs when the PR has a spec or linked issue. Moving it to Correctness would make it run on every review; I kept it with Spec because "the change replaced X" is usually stated in the spec. Not yet validated by a /pr-review 4381 run against the manual findings.

🤖 Generated with Claude Code

@jarhodes314
jarhodes314 force-pushed the jarhodes314/pr-review-spec-axis-and-untrusted-input branch from 20aaa89 to 619796c Compare October 28, 2026 15:56
@jarhodes314 jarhodes314 changed the title docs: add untrusted-input and constructor-validation standards, widen pr-review spec axis chore: add untrusted-input and constructor-validation standards, widen pr-review spec axis Oct 28, 2026
@jarhodes314 jarhodes314 added theme:developer-exp Theme: improve developer experience skip-release-notes Don't include the ticket in the auto generated release notes labels Oct 28, 2026
@codecov

codecov Bot commented Oct 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread CODING_STANDARDS.md Resolved
Comment thread CODING_STANDARDS.md Resolved
@jarhodes314
jarhodes314 force-pushed the jarhodes314/pr-review-spec-axis-and-untrusted-input branch from a0eb40f to fe8cc8b Compare November 10, 2026 10:08
@jarhodes314
jarhodes314 requested review from a team and Bravo555 as code owners November 10, 2026 10:08
@jarhodes314
jarhodes314 deployed to Test Pull Request November 10, 2026 10:08 — with GitHub Actions Active
… pr-review spec axis

CODING_STANDARDS.md gains a rule to validate in the type's own
constructor and an "Untrusted input" subsection. The pr-review Spec
prompt now also reports requirements that are the wrong solution and
searches for stale uses of a replaced pattern outside the diff.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Signed-off-by: James Rhodes <[email protected]>
@jarhodes314
jarhodes314 force-pushed the jarhodes314/pr-review-spec-axis-and-untrusted-input branch from fe8cc8b to 8446b19 Compare November 10, 2026 10:11
@jarhodes314
jarhodes314 deployed to Test Pull Request November 10, 2026 10:11 — with GitHub Actions Active
@github-actions

github-actions Bot commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
1055 4 8 1059 99.62 3h13m12.275147s

Failed Tests

Name Message ⏱️ Duration Suite
Runs the shipped restart workflow through the plugin of the service type Parent suite setup failed: tedge connect c8y returned an unexpected exit code stdout: stderr: Connecting to Cumulocity: mapper configuration file: /etc/tedge/mappers/c8y/mapper.toml device id: TST_restore_zesty_gear cloud profile: <none> cloud host: qaenvironment.eu-latest.cumulocity.com:8883 auth type: Certificate certificate file: /etc/tedge/device-certs/tedge-certificate.pem cryptoki: off bridge: built-in service manager: systemd mosquitto version: 2.1.2 proxy: Not configured Updating bridge rule templates... ✓ Creating device in Cumulocity cloud... ✓ Restarting mosquitto... ✓ Waiting for mosquitto to be listening for connections... ✓ [ Message content over the limit has been removed. ] error: Failed to verify device is connected to Cumulocity: Connection to Cumulocity is not healthy Connection test failed, attempt 2 of 5 Verifying device is connected to cloud... ✗ error: Failed to verify device is connected to Cumulocity: Connection to Cumulocity is not healthy Connection test failed, attempt 3 of 5 Verifying device is connected to cloud... ✗ error: Failed to verify device is connected to Cumulocity: Connection to Cumulocity is not healthy Connection test failed, attempt 4 of 5 Verifying device is connected to cloud... ✗ error: Failed to verify device is connected to Cumulocity: Connection to Cumulocity is not healthy warning: Bridge has been configured, but Cumulocity connection check failed. Enabling tedge-agent... ✓ error: Connection check failed [RETRY] FAIL on 3. retry. 0.022 s Service Plugin
Runs a custom action through the shipped workflow and the plugin of the service type Parent suite setup failed: tedge connect c8y returned an unexpected exit code stdout: stderr: Connecting to Cumulocity: mapper configuration file: /etc/tedge/mappers/c8y/mapper.toml device id: TST_restore_zesty_gear cloud profile: <none> cloud host: qaenvironment.eu-latest.cumulocity.com:8883 auth type: Certificate certificate file: /etc/tedge/device-certs/tedge-certificate.pem cryptoki: off bridge: built-in service manager: systemd mosquitto version: 2.1.2 proxy: Not configured Updating bridge rule templates... ✓ Creating device in Cumulocity cloud... ✓ Restarting mosquitto... ✓ Waiting for mosquitto to be listening for connections... ✓ [ Message content over the limit has been removed. ] error: Failed to verify device is connected to Cumulocity: Connection to Cumulocity is not healthy Connection test failed, attempt 2 of 5 Verifying device is connected to cloud... ✗ error: Failed to verify device is connected to Cumulocity: Connection to Cumulocity is not healthy Connection test failed, attempt 3 of 5 Verifying device is connected to cloud... ✗ error: Failed to verify device is connected to Cumulocity: Connection to Cumulocity is not healthy Connection test failed, attempt 4 of 5 Verifying device is connected to cloud... ✗ error: Failed to verify device is connected to Cumulocity: Connection to Cumulocity is not healthy warning: Bridge has been configured, but Cumulocity connection check failed. Enabling tedge-agent... ✓ error: Connection check failed [RETRY] FAIL on 3. retry. 0.019 s Service Plugin
Takes the service name from the operation Parent suite setup failed: tedge connect c8y returned an unexpected exit code stdout: stderr: Connecting to Cumulocity: mapper configuration file: /etc/tedge/mappers/c8y/mapper.toml device id: TST_restore_zesty_gear cloud profile: <none> cloud host: qaenvironment.eu-latest.cumulocity.com:8883 auth type: Certificate certificate file: /etc/tedge/device-certs/tedge-certificate.pem cryptoki: off bridge: built-in service manager: systemd mosquitto version: 2.1.2 proxy: Not configured Updating bridge rule templates... ✓ Creating device in Cumulocity cloud... ✓ Restarting mosquitto... ✓ Waiting for mosquitto to be listening for connections... ✓ [ Message content over the limit has been removed. ] error: Failed to verify device is connected to Cumulocity: Connection to Cumulocity is not healthy Connection test failed, attempt 2 of 5 Verifying device is connected to cloud... ✗ error: Failed to verify device is connected to Cumulocity: Connection to Cumulocity is not healthy Connection test failed, attempt 3 of 5 Verifying device is connected to cloud... ✗ error: Failed to verify device is connected to Cumulocity: Connection to Cumulocity is not healthy Connection test failed, attempt 4 of 5 Verifying device is connected to cloud... ✗ error: Failed to verify device is connected to Cumulocity: Connection to Cumulocity is not healthy warning: Bridge has been configured, but Cumulocity connection check failed. Enabling tedge-agent... ✓ error: Connection check failed [RETRY] FAIL on 3. retry. 0.019 s Service Plugin
Takes the service name and type from the registered entity when the operation gives none Parent suite setup failed: tedge connect c8y returned an unexpected exit code stdout: stderr: Connecting to Cumulocity: mapper configuration file: /etc/tedge/mappers/c8y/mapper.toml device id: TST_restore_zesty_gear cloud profile: <none> cloud host: qaenvironment.eu-latest.cumulocity.com:8883 auth type: Certificate certificate file: /etc/tedge/device-certs/tedge-certificate.pem cryptoki: off bridge: built-in service manager: systemd mosquitto version: 2.1.2 proxy: Not configured Updating bridge rule templates... ✓ Creating device in Cumulocity cloud... ✓ Restarting mosquitto... ✓ Waiting for mosquitto to be listening for connections... ✓ [ Message content over the limit has been removed. ] error: Failed to verify device is connected to Cumulocity: Connection to Cumulocity is not healthy Connection test failed, attempt 2 of 5 Verifying device is connected to cloud... ✗ error: Failed to verify device is connected to Cumulocity: Connection to Cumulocity is not healthy Connection test failed, attempt 3 of 5 Verifying device is connected to cloud... ✗ error: Failed to verify device is connected to Cumulocity: Connection to Cumulocity is not healthy Connection test failed, attempt 4 of 5 Verifying device is connected to cloud... ✗ error: Failed to verify device is connected to Cumulocity: Connection to Cumulocity is not healthy warning: Bridge has been configured, but Cumulocity connection check failed. Enabling tedge-agent... ✓ error: Connection check failed [RETRY] FAIL on 3. retry. 0.021 s Service Plugin

This branch had an error being deployed

1 failed and 1 active deployments
Test Auto — 8446b197 Deployed Nov 11, 2026 by jarhodes314 via Test x86_64 #8882
Test Pull Request — 8446b197 Deployed Nov 10, 2026 by jarhodes314 via Approve #8882
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-release-notes Don't include the ticket in the auto generated release notes theme:developer-exp Theme: improve developer experience

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants